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: gatt: Fix ATT Read By Type by DB change unaware client #75720

Merged
merged 1 commit into from
Aug 27, 2024
Merged

bluetooth: gatt: Fix ATT Read By Type by DB change unaware client #75720

merged 1 commit into from
Aug 27, 2024

Conversation

sjanc
Copy link
Collaborator

@sjanc sjanc commented Jul 10, 2024 •
edited by nashif
Loading

When change unaware client send ATT request it shall get Database Out of Sync error. Reading GATT database hash is an exception here.

This was affecting GATT/SR/GAS/BV-05-C qualification test case.

Fixes #77088

Thalley
Thalley reviewed Jul 10, 2024
View reviewed changes
subsys/bluetooth/host/att.c Outdated
@@ -1548,6 +1548,19 @@ static uint8_t att_read_type_req(struct bt_att_chan *chan, struct net_buf *buf)
return 0;
}

/* Reading Database Hash is special as it is required to make client
Copy link
Collaborator

Choose a reason for hiding this comment

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

In all the other cases we have this part

	if (!bt_gatt_change_aware(chan->att->conn, true)) {
		if (!atomic_test_and_set_bit(chan->flags, ATT_OUT_OF_SYNC_SENT)) {
			return BT_ATT_ERR_DB_OUT_OF_SYNC;
		} else {
			return 0;
		}
	}

in the rsp functions.

To be consistent, shouldn't we simply add the above to att_read_type_rsp?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the point of this PR is adding the additional check if (bt_uuid_cmp(&u.uuid, BT_UUID_GATT_DB_HASH) != 0) {, then shouldn't that check be added to all read handlers? What if the client reads the database with BT_ATT_OP_READ_REQ instead of BT_ATT_OP_READ_TYPE_REQ?

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 not very consistent as some (att_read_mult_req, att_read_mult_vl_req) are handled in req handlers, but I can do this in rsp too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the point of this PR is adding the additional check if (bt_uuid_cmp(&u.uuid, BT_UUID_GATT_DB_HASH) != 0) {, then shouldn't that check be added to all read handlers? What if the client reads the database with BT_ATT_OP_READ_REQ instead of BT_ATT_OP_READ_TYPE_REQ?

Change is to handle GATT Read Using Characteristic UUID sub-procedure when client is change unaware,
last step in mentioned test is:
"16. The Lower Tester executes the GATT Read Using Characteristic UUID sub-procedure request to
the IUT with the Attribute Type not set to <> or <> and an Attribute
Handle range other than 0x0001 to 0xFFFF and expects the IUT to return the Database Out of
Sync error."

And DB Hash characteristic shall be read using this procedure, hence this UUID check
Core Spec 5.4 Vol 3 Part G 7.3 DATABASE HASH:
"In order to read the value of this characteristic the client shall always use the
GATT Read Using Characteristic UUID sub-procedure. The Starting Handle
should be set to 0x0001 and the Ending Handle should be set to 0xFFFF."

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not very consistent as some (att_read_mult_req, att_read_mult_vl_req) are handled in req handlers, but I can do this in rsp too

Planning any updates on this?
It's unclear to me whether this PR needs a change or not

@jhedberg
Copy link
Member

@sjanc should this go into 3.7? If so, it likely requires a matching bug report (as well as the respective milestone set).

@carlescufi carlescufi added the bug The issue is a bug, or the PR is fixing a bug label Jul 11, 2024
@MaureenHelm
Copy link
Member

@sjanc @jhedberg ping

@jhedberg
Copy link
Member

@sjanc should this go into 3.7? If so, it likely requires a matching bug report (as well as the respective milestone set).

@sjanc I'm still waiting for a reply to this.

@sjanc sjanc added the area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests label Aug 23, 2024
@sjanc
Copy link
Collaborator Author

sjanc commented Aug 23, 2024

If qualification fixes are to be backported to 3.7 than yes.

Note that this is stil pending review, preferably from someone familiar with robust GATT caching

@jhedberg jhedberg added the backport v3.7-branch Request backport to the v3.7-branch label Aug 23, 2024
@jhedberg
Copy link
Member

If qualification fixes are to be backported to 3.7 than yes.

They are. I added the necessary label now.

Note that this is stil pending review, preferably from someone familiar with robust GATT caching

Ok. I'll wait with my approval until then.

@sjanc
Copy link
Collaborator Author

sjanc commented Aug 23, 2024

#AutoPTS run zephyr GATT/SR

@codecoup-tester
Copy link

Scheduled PR #75720 (comment), board: nrf52, estimated start time: 11:37:46, test case count: 89, estimated duration: 0:41:51

Test cases to be runGATT/SR/GAC/BV-01-C
GATT/SR/GAC/BI-01-C
GATT/SR/GAD/BV-01-C
GATT/SR/GAD/BV-02-C
GATT/SR/GAD/BV-03-C
GATT/SR/GAD/BV-04-C
GATT/SR/GAD/BV-05-C
GATT/SR/GAD/BV-06-C
GATT/SR/GAR/BV-01-C
GATT/SR/GAR/BI-01-C
GATT/SR/GAR/BI-02-C
GATT/SR/GAR/BI-03-C
GATT/SR/GAR/BI-04-C
GATT/SR/GAR/BI-05-C
GATT/SR/GAR/BV-03-C
GATT/SR/GAR/BI-06-C
GATT/SR/GAR/BI-07-C
GATT/SR/GAR/BI-08-C
GATT/SR/GAR/BI-09-C
GATT/SR/GAR/BI-10-C
GATT/SR/GAR/BI-11-C
GATT/SR/GAR/BV-04-C
GATT/SR/GAR/BI-12-C
GATT/SR/GAR/BI-13-C
GATT/SR/GAR/BI-14-C
GATT/SR/GAR/BI-15-C
GATT/SR/GAR/BI-16-C
GATT/SR/GAR/BI-17-C
GATT/SR/GAR/BV-05-C
GATT/SR/GAR/BI-18-C
GATT/SR/GAR/BI-19-C
GATT/SR/GAR/BI-20-C
GATT/SR/GAR/BI-21-C
GATT/SR/GAR/BI-22-C
GATT/SR/GAR/BV-06-C
GATT/SR/GAR/BV-07-C
GATT/SR/GAR/BV-08-C
GATT/SR/GAR/BV-09-C
GATT/SR/GAR/BV-11-C
GATT/SR/GAR/BV-12-C
GATT/SR/GAR/BI-36-C
GATT/SR/GAR/BI-38-C
GATT/SR/GAR/BI-40-C
GATT/SR/GAR/BI-42-C
GATT/SR/GAR/BI-44-C
GATT/SR/GAR/BI-45-C
GATT/SR/GAW/BV-01-C
GATT/SR/GAW/BV-02-C
GATT/SR/GAW/BI-01-C
GATT/SR/GAW/BV-03-C
GATT/SR/GAW/BI-02-C
GATT/SR/GAW/BI-03-C
GATT/SR/GAW/BI-04-C
GATT/SR/GAW/BI-05-C
GATT/SR/GAW/BI-06-C
GATT/SR/GAW/BV-05-C
GATT/SR/GAW/BI-07-C
GATT/SR/GAW/BI-08-C
GATT/SR/GAW/BI-09-C
GATT/SR/GAW/BI-11-C
GATT/SR/GAW/BI-12-C
GATT/SR/GAW/BI-13-C
GATT/SR/GAW/BV-06-C
GATT/SR/GAW/BV-10-C
GATT/SR/GAW/BV-11-C
GATT/SR/GAW/BV-07-C
GATT/SR/GAW/BV-08-C
GATT/SR/GAW/BV-09-C
GATT/SR/GAW/BI-32-C
GATT/SR/GAW/BI-33-C
GATT/SR/GAW/BV-12-C
GATT/SR/GAW/BV-13-C
GATT/SR/GAW/BV-14-C
GATT/SR/GAW/BI-38-C
GATT/SR/GAN/BV-01-C
GATT/SR/GAN/BV-02-C
GATT/SR/GAI/BV-01-C
GATT/SR/GAS/BV-01-C
GATT/SR/GAS/BV-02-C
GATT/SR/GAS/BV-03-C
GATT/SR/GAS/BV-04-C
GATT/SR/GAS/BV-05-C
GATT/SR/GAS/BV-06-C
GATT/SR/GAS/BV-07-C
GATT/SR/GAS/BV-08-C
GATT/SR/GAT/BV-01-C
GATT/SR/UNS/BI-01-C
GATT/SR/UNS/BI-02-C
GATT/SR/GPM/BV-05-C

@codecoup-tester
Copy link

AutoPTS Bot results:
No failed test found.

Successful testsGATT GATT/SR/GAC/BI-01-C PASS
GATT GATT/SR/GAC/BV-01-C PASS
GATT GATT/SR/GAD/BV-01-C PASS
GATT GATT/SR/GAD/BV-02-C PASS
GATT GATT/SR/GAD/BV-03-C PASS
GATT GATT/SR/GAD/BV-04-C PASS
GATT GATT/SR/GAD/BV-05-C PASS
GATT GATT/SR/GAD/BV-06-C PASS
GATT GATT/SR/GAI/BV-01-C PASS
GATT GATT/SR/GAN/BV-01-C PASS
GATT GATT/SR/GAN/BV-02-C PASS
GATT GATT/SR/GAR/BI-01-C PASS
GATT GATT/SR/GAR/BI-02-C PASS
GATT GATT/SR/GAR/BI-03-C PASS
GATT GATT/SR/GAR/BI-04-C PASS
GATT GATT/SR/GAR/BI-05-C PASS
GATT GATT/SR/GAR/BI-06-C PASS
GATT GATT/SR/GAR/BI-07-C PASS
GATT GATT/SR/GAR/BI-08-C PASS
GATT GATT/SR/GAR/BI-09-C PASS
GATT GATT/SR/GAR/BI-10-C PASS
GATT GATT/SR/GAR/BI-11-C PASS
GATT GATT/SR/GAR/BI-12-C PASS
GATT GATT/SR/GAR/BI-13-C PASS
GATT GATT/SR/GAR/BI-14-C PASS
GATT GATT/SR/GAR/BI-15-C PASS
GATT GATT/SR/GAR/BI-16-C PASS
GATT GATT/SR/GAR/BI-17-C PASS
GATT GATT/SR/GAR/BI-18-C PASS
GATT GATT/SR/GAR/BI-19-C PASS
GATT GATT/SR/GAR/BI-20-C PASS
GATT GATT/SR/GAR/BI-21-C PASS
GATT GATT/SR/GAR/BI-22-C PASS
GATT GATT/SR/GAR/BI-36-C PASS
GATT GATT/SR/GAR/BI-38-C PASS
GATT GATT/SR/GAR/BI-40-C PASS
GATT GATT/SR/GAR/BI-42-C PASS
GATT GATT/SR/GAR/BI-44-C PASS
GATT GATT/SR/GAR/BI-45-C PASS
GATT GATT/SR/GAR/BV-01-C PASS
GATT GATT/SR/GAR/BV-03-C PASS
GATT GATT/SR/GAR/BV-04-C PASS
GATT GATT/SR/GAR/BV-05-C PASS
GATT GATT/SR/GAR/BV-06-C PASS
GATT GATT/SR/GAR/BV-07-C PASS
GATT GATT/SR/GAR/BV-08-C PASS
GATT GATT/SR/GAR/BV-09-C PASS
GATT GATT/SR/GAR/BV-11-C PASS
GATT GATT/SR/GAR/BV-12-C PASS
GATT GATT/SR/GAS/BV-01-C PASS
GATT GATT/SR/GAS/BV-02-C PASS
GATT GATT/SR/GAS/BV-03-C PASS
GATT GATT/SR/GAS/BV-04-C PASS
GATT GATT/SR/GAS/BV-05-C PASS
GATT GATT/SR/GAS/BV-06-C PASS
GATT GATT/SR/GAS/BV-07-C PASS
GATT GATT/SR/GAS/BV-08-C PASS (2)
GATT GATT/SR/GAT/BV-01-C PASS
GATT GATT/SR/GAW/BI-01-C PASS
GATT GATT/SR/GAW/BI-02-C PASS
GATT GATT/SR/GAW/BI-03-C PASS
GATT GATT/SR/GAW/BI-04-C PASS
GATT GATT/SR/GAW/BI-05-C PASS
GATT GATT/SR/GAW/BI-06-C PASS
GATT GATT/SR/GAW/BI-07-C PASS
GATT GATT/SR/GAW/BI-08-C PASS
GATT GATT/SR/GAW/BI-09-C PASS
GATT GATT/SR/GAW/BI-11-C PASS
GATT GATT/SR/GAW/BI-12-C PASS
GATT GATT/SR/GAW/BI-13-C PASS
GATT GATT/SR/GAW/BI-32-C PASS
GATT GATT/SR/GAW/BI-33-C PASS
GATT GATT/SR/GAW/BI-38-C PASS
GATT GATT/SR/GAW/BV-01-C PASS
GATT GATT/SR/GAW/BV-02-C PASS
GATT GATT/SR/GAW/BV-03-C PASS
GATT GATT/SR/GAW/BV-05-C PASS
GATT GATT/SR/GAW/BV-06-C PASS
GATT GATT/SR/GAW/BV-07-C PASS
GATT GATT/SR/GAW/BV-08-C PASS
GATT GATT/SR/GAW/BV-09-C PASS
GATT GATT/SR/GAW/BV-10-C PASS
GATT GATT/SR/GAW/BV-11-C PASS
GATT GATT/SR/GAW/BV-12-C PASS
GATT GATT/SR/GAW/BV-13-C PASS
GATT GATT/SR/GAW/BV-14-C PASS
GATT GATT/SR/GPM/BV-05-C PASS
GATT GATT/SR/UNS/BI-01-C PASS
GATT GATT/SR/UNS/BI-02-C PASS

@sjanc
bluetooth: gatt: Fix ATT Read By Type by DB change unaware client
ff9a2fd 
When change unaware client send ATT request it shall get Database
Out of Sync error. Reading GATT database hash is an exception here.

This was affecting GATT/SR/GAS/BV-05-C qualification test case.

Signed-off-by: Szymon Janc <szymon.janc@codecoup.pl>
@sjanc sjanc force-pushed the gatt_db_hash_read_type branch from ced6072 to ff9a2fd Compare August 27, 2024 09:27
Thalley
Thalley approved these changes Aug 27, 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.

The changes look good. Possible extension to reject invalid read requests from other types may be needed

subsys/bluetooth/host/att.c
Comment on lines +1555 to +1559
* GATT client shall always use GATT Read Using Characteristic UUID sub-procedure for
* reading Database Hash
* (Core Specification 5.4 Vol 3. Part G. 7.3 Databse Hash)
*/
if (bt_uuid_cmp(&u.uuid, BT_UUID_GATT_DB_HASH) != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, this made me think of whether we can/should reject reads to BT_UUID_GATT_DB_HASH from the other read requests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

other reads (and also wrtite reqs) are already handling it (that if bt_gatt_change_aware() blocks are doing that)

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are just blocking reading when the database is out of sync though, right?

The core spec states that the client shall read the database sync with the read by type request (read by UUID). That means that even if the client is not out of sync, it shall not read the characteristic with e.g. a regular read request. Arguably that is a requirement on the client side, but we could (not sure if we should) block reads of that characteristic if not with read by type

@nashif nashif merged commit 77cbd27 into zephyrproject-rtos:main Aug 27, 2024
25 checks passed
@zephyrbot zephyrbot mentioned this pull request Aug 27, 2024
Merged
@sjanc sjanc deleted the gatt_db_hash_read_type branch August 27, 2024 14:03
yvanderv pushed a commit to nxp-zephyr/nxp-zephyr that referenced this pull request Dec 12, 2024
@sjanc @danieldegrasse
[nxp fromlist] bluetooth: gatt: Fix ATT Read By Type by DB change una…
da6e035 
…ware client

When change unaware client send ATT request it shall get Database
Out of Sync error. Reading GATT database hash is an exception here.

This was affecting GATT/SR/GAS/BV-05-C qualification test case.

Signed-off-by: Szymon Janc <szymon.janc@codecoup.pl>

(cherry picked from upstream PR: zephyrproject-rtos/zephyr#75720)
yvanderv pushed a commit to nxp-zephyr/nxp-zephyr that referenced this pull request Dec 12, 2024
@sjanc @danieldegrasse
[nxp fromlist] bluetooth: gatt: Fix ATT Read By Type by DB change una…
665ae2a 
…ware client

When change unaware client send ATT request it shall get Database
Out of Sync error. Reading GATT database hash is an exception here.

This was affecting GATT/SR/GAS/BV-05-C qualification test case.

Signed-off-by: Szymon Janc <szymon.janc@codecoup.pl>

(cherry picked from upstream PR: zephyrproject-rtos/zephyr#75720)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thalley Thalley Thalley approved these changes

@jhedberg jhedberg jhedberg approved these changes

@alwa-nordic alwa-nordic Awaiting requested review from alwa-nordic

@hermabe hermabe Awaiting requested review from hermabe

@jori-nordic jori-nordic Awaiting requested review from jori-nordic

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

Assignees

@jhedberg jhedberg

@jori-nordic jori-nordic

Labels
area: Bluetooth Host area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

net: buf: Pre-initialize user_data on net_buf_alloc()
9 participants
@sjanc @jhedberg @MaureenHelm @codecoup-tester @Thalley @nashif @carlescufi @zephyrbot @jori-nordic