Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bluetooth: L2CAP: Set NULL callback for PDUs #76489

Merged
merged 6 commits into from
Aug 1, 2024
Merged

Bluetooth: L2CAP: Set NULL callback for PDUs #76489

merged 6 commits into from
Aug 1, 2024

Conversation

jori-nordic
Copy link
Collaborator

@jori-nordic jori-nordic commented Jul 30, 2024 •
edited
Loading

It was not being set, and thus if the user_data contained garbage from
before, then conn.c would attempt to call that garbage.

Static channels don't have this issue, as every "SDU" fits into one PDU.

Watch the test fail:

  • Add CONFIG_NO_RUNTIME_CHECKS=y to prj.conf
  • Revert the last commit "Bluetooth: L2CAP: Set NULL callback for PDUs"

@jori-nordic
Bluetooth: host: add more info to conn.c log
9130596 
Print user_data and callback pointers.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic jori-nordic added bug The issue is a bug, or the PR is fixing a bug area: Bluetooth Host labels Jul 30, 2024
@zephyrbot zephyrbot added platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Bluetooth labels Jul 30, 2024
@jori-nordic jori-nordic mentioned this pull request Jul 30, 2024
Closed
@jori-nordic jori-nordic force-pushed the all-your-data-is-belong-to-us branch 2 times, most recently from 8b4da08 to a1a0e77 Compare July 30, 2024 15:32
@jori-nordic
Bluetooth: allow compiling host with CONFIG_NO_RUNTIME_CHECKS
da71325 
Werror fails the build on that function.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic jori-nordic force-pushed the all-your-data-is-belong-to-us branch from a1a0e77 to 0ad4d6b Compare July 30, 2024 15:34
Thalley
Thalley requested changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing initialization of loop variable needs to be fixed; the others are optional

include/zephyr/bluetooth/l2cap.h Outdated Show resolved Hide resolved
subsys/bluetooth/host/conn.c Show resolved Hide resolved
subsys/bluetooth/host/l2cap.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/l2cap.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/l2cap.c Outdated Show resolved Hide resolved
@jori-nordic
Bluetooth: L2CAP: Mark user_data as owned by the stack
43dea71 
Storing stuff in user_data? That's a paddlin'

We have been debugging issue after issue because ownership of this
"user" data is not clearly defined. Now it is. L2CAP owns the user_data
field entirely, as soon as `send()` is called.

Also add a warning and retval using CHECKIF.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic jori-nordic force-pushed the all-your-data-is-belong-to-us branch 2 times, most recently from 85be2d9 to 66401f0 Compare July 31, 2024 08:32
jori-nordic
jori-nordic commented Jul 31, 2024
View reviewed changes
tests/bsim/bluetooth/host/l2cap/userdata/src/main_l2cap_userdata.c
@@ -14,8 +14,15 @@ extern enum bst_result_t bst_result;
static struct bt_conn *default_conn;

#define PSM 0x80
#define DATA_SIZE 500
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to make sure we have many K-Frames (PDUs) to send an SDU. Since the bug was discovered with K-frames in the middle of an SDU.

jori-nordic and others added 3 commits July 31, 2024 11:03
@jori-nordic
Bluetooth: adapt l2cap/userdata test
adeebea 
Modify the test so it checks that user_data is used and then cleared by
the stack.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic
Bluetooth: remove forgotten TODO
d728c8d 
It's already done.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic
Bluetooth: L2CAP: Set NULL callback for PDUs
e21a121 
It was not being set, and thus if the user_data contained garbage from
before, then conn.c would attempt to call that garbage.

Static channels don't have this issue, as every "SDU" fits into one PDU.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
Co-authored-by: Huajiang Zheng <nxf88597@lsv051208.swis.nl-cdc01.nxp.com>
@jori-nordic jori-nordic force-pushed the all-your-data-is-belong-to-us branch from 66401f0 to e21a121 Compare July 31, 2024 09:03
@jori-nordic
Copy link
Collaborator Author

Sneak peek: @alwa-nordic and I are trying to get rid of user_data entirely jori-nordic@295fc08

@fabiobaltieri fabiobaltieri merged commit 1c65103 into zephyrproject-rtos:main Aug 1, 2024
25 checks passed
@jhedberg jhedberg mentioned this pull request Aug 6, 2024
Closed
@jori-nordic jori-nordic added the backport v3.7-branch Request backport to the v3.7-branch label Aug 7, 2024
@zephyrbot zephyrbot mentioned this pull request Aug 7, 2024
Merged
@jori-nordic jori-nordic linked an issue Aug 7, 2024 that may be closed by this pull request
Sending Bluetooth L2CAP messages after reconnecting sometimes leads to crashes due to uninitalized net_buf callback #76738
Closed
jori-nordic added a commit to jori-nordic/zephyr that referenced this pull request Aug 15, 2024
@jori-nordic
Bluetooth: L2CAP: Downgrade user_data API error to a warning
85e9b73 
When user_data is not zeroed-out, the API returns an error. Downgrade
the API error to a warning log instead.

Introducing this check (zephyrproject-rtos#76489) broke a few PTS tests, as user_data is
not initialized by `net_buf_alloc()`. Doing so is in discussion (see

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
jori-nordic added a commit to jori-nordic/zephyr that referenced this pull request Aug 15, 2024
@jori-nordic
Bluetooth: L2CAP: Downgrade user_data API error to a warning
fec2036 
When user_data is not zeroed-out, the API returns an error. Downgrade
the API error to a warning log instead.

Introducing this check (zephyrproject-rtos#76489) broke a few PTS tests, as user_data is
not initialized by `net_buf_alloc()`. Doing so is in discussion (see

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
jori-nordic added a commit to jori-nordic/zephyr that referenced this pull request Aug 15, 2024
@jori-nordic
Bluetooth: L2CAP: Downgrade user_data API error to a warning
0e1c346 
When user_data is not zeroed-out, the API returns an error. Downgrade
the API error to a warning log instead.

Introducing this check (zephyrproject-rtos#76489) broke a few PTS tests, as user_data is
not initialized by `net_buf_alloc()`. Doing so is in discussion:

zephyrproject-rtos#77088

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
@jori-nordic jori-nordic mentioned this pull request Aug 15, 2024
Merged
nashif pushed a commit that referenced this pull request Aug 19, 2024
@jori-nordic @nashif
Bluetooth: L2CAP: Downgrade user_data API error to a warning
6fa6c4c 
When user_data is not zeroed-out, the API returns an error. Downgrade
the API error to a warning log instead.

Introducing this check (#76489) broke a few PTS tests, as user_data is
not initialized by `net_buf_alloc()`. Doing so is in discussion:

#77088

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
zephyrbot pushed a commit that referenced this pull request Aug 19, 2024
@jori-nordic @github-actions
Bluetooth: L2CAP: Downgrade user_data API error to a warning
0bda1b0 
When user_data is not zeroed-out, the API returns an error. Downgrade
the API error to a warning log instead.

Introducing this check (#76489) broke a few PTS tests, as user_data is
not initialized by `net_buf_alloc()`. Doing so is in discussion:

#77088

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
(cherry picked from commit 6fa6c4c)
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this pull request Aug 29, 2024
@jori-nordic
Bluetooth: L2CAP: Downgrade user_data API error to a warning
bd31968 
When user_data is not zeroed-out, the API returns an error. Downgrade
the API error to a warning log instead.

Introducing this check (zephyrproject-rtos#76489) broke a few PTS tests, as user_data is
not initialized by `net_buf_alloc()`. Doing so is in discussion:

zephyrproject-rtos#77088

(cherry picked from commit 6fa6c4c)

Original-Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>
GitOrigin-RevId: 6fa6c4c
Cr-Build-Id: 8739091610579900001
Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8739091610579900001
Copybot-Job-Name: zephyr-main-copybot-downstream
Change-Id: Ic115fe0bdfe6595a874c7782cd8385c49f99405c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5799351
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
Reviewed-by: Eric Yilun Lin <yllin@google.com>
Commit-Queue: Eric Yilun Lin <yllin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhedberg jhedberg jhedberg approved these changes

@Thalley Thalley Thalley approved these changes

@alwa-nordic alwa-nordic alwa-nordic approved these changes

@aescolar aescolar Awaiting requested review from aescolar

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@hermabe hermabe Awaiting requested review from hermabe

@sjanc sjanc Awaiting requested review from sjanc

@theob-pro theob-pro Awaiting requested review from theob-pro

@Vudentz Vudentz Awaiting requested review from Vudentz

Assignees

@jhedberg jhedberg

Labels
area: Bluetooth Host area: Bluetooth backport v3.7-branch Request backport to the v3.7-branch bug The issue is a bug, or the PR is fixing a bug platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim
Projects
None yet
Milestone
No milestone
6 participants
@jori-nordic @jhedberg @Thalley @alwa-nordic @fabiobaltieri @zephyrbot