I'm working on a tool for (reliably) transferring diffs across versions of a given C code. To evaluate my tool, I have been comparing the output of my tool against the manual backports in the stable branch.
In the process, I have found some oddities in some backported patches which, I believe, are incorrect. In all cases, the root cause seems to be that `patch` was able to apply a backported diff after some fuzzy matching but did so at a wrong location (IMHO). Below is an example. I can report the others if there is indeed a problem.
On the branch stable/linux-4.4.y, the upstream patch
b560a208cda0297fef6ff85bbfd58a8f0a52a543 Bluetooth: MGMT: Fix not checking if BT_HS is enabled
is backported as commit
5abe9f99f5129bee5492072ff76b91ec4fad485b.
If we look at both commits we have:
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% Upstream b560a208cda0297fef6ff85bbfd58a8f0a52a543 %%%%%%%%%
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 0b711ad..12d7b36 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c [...] @@ -1817,6 +1818,10 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len) bt_dev_dbg(hdev, "sock %p", sk); + if (!IS_ENABLED(CONFIG_BT_HS)) + return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, + MGMT_STATUS_NOT_SUPPORTED); + status = mgmt_bredr_support(hdev); if (status) return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, status);
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% Backported 5abe9f99f5129bee5492072ff76b91ec4fad485b %%%%%%%%%
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index ecc3da6..ee761fb 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c [...] @@ -2281,6 +2282,10 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data, BT_DBG("request for %s", hdev->name); + if (!IS_ENABLED(CONFIG_BT_HS)) + return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, + MGMT_STATUS_NOT_SUPPORTED); + status = mgmt_bredr_support(hdev); if (status) return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
I suspect that this does *not* reflect the intent of the orignal patch (which was addressing an issue in `set_hs`) whereas, here, the backported patch is somewhat arbitrarily modifying `set_link_security` while `set_hs` remains as-is.
Is this indeed an issue? Should I report on the other cases as well?
- Guillaume Bertholon
On Tue, Jan 25, 2022 at 05:09:46PM +0100, Guillaume Bertholon wrote:
I'm working on a tool for (reliably) transferring diffs across versions of a given C code. To evaluate my tool, I have been comparing the output of my tool against the manual backports in the stable branch.
In the process, I have found some oddities in some backported patches which, I believe, are incorrect. In all cases, the root cause seems to be that `patch` was able to apply a backported diff after some fuzzy matching but did so at a wrong location (IMHO). Below is an example. I can report the others if there is indeed a problem.
On the branch stable/linux-4.4.y, the upstream patch
b560a208cda0297fef6ff85bbfd58a8f0a52a543 Bluetooth: MGMT: Fix not checking if BT_HS is enabled
is backported as commit
5abe9f99f5129bee5492072ff76b91ec4fad485b.
If we look at both commits we have:
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% Upstream b560a208cda0297fef6ff85bbfd58a8f0a52a543 %%%%%%%%%
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 0b711ad..12d7b36 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c [...] @@ -1817,6 +1818,10 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len) bt_dev_dbg(hdev, "sock %p", sk); + if (!IS_ENABLED(CONFIG_BT_HS)) + return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, + MGMT_STATUS_NOT_SUPPORTED);
status = mgmt_bredr_support(hdev); if (status) return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, status);
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% Backported 5abe9f99f5129bee5492072ff76b91ec4fad485b %%%%%%%%%
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index ecc3da6..ee761fb 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c [...] @@ -2281,6 +2282,10 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data, BT_DBG("request for %s", hdev->name); + if (!IS_ENABLED(CONFIG_BT_HS)) + return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, + MGMT_STATUS_NOT_SUPPORTED);
status = mgmt_bredr_support(hdev); if (status) return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
I suspect that this does *not* reflect the intent of the orignal patch (which was addressing an issue in `set_hs`) whereas, here, the backported patch is somewhat arbitrarily modifying `set_link_security` while `set_hs` remains as-is.
Is this indeed an issue? Should I report on the other cases as well?
Yes, this looks like an incorrect backport, nice catch!
Can you send a fixup patch for this so that I can apply it and give you the correct credit for finding and fixing it?
Also, yes, if you have other reports of this, it would be great to let us know.
thanks
greg k-h
The upstream commit b560a208cda0 ("Bluetooth: MGMT: Fix not checking if BT_HS is enabled") inserted a new check in the `set_hs` function. However, its backported version in stable (commit 5abe9f99f512 ("Bluetooth: MGMT: Fix not checking if BT_HS is enabled")), added the check in `set_link_security` instead.
This patch restores the intent of the upstream commit by moving back the BT_HS check to `set_hs`.
Fixes: 5abe9f99f512 ("Bluetooth: MGMT: Fix not checking if BT_HS is enabled") Signed-off-by: Guillaume Bertholon guillaume.bertholon@ens.fr --- net/bluetooth/mgmt.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 4a95c89..621329c 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -2285,10 +2285,6 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
- if (!IS_ENABLED(CONFIG_BT_HS)) - return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, - MGMT_STATUS_NOT_SUPPORTED); - status = mgmt_bredr_support(hdev); if (status) return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY, @@ -2438,6 +2434,10 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
BT_DBG("request for %s", hdev->name);
+ if (!IS_ENABLED(CONFIG_BT_HS)) + return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, + MGMT_STATUS_NOT_SUPPORTED); + status = mgmt_bredr_support(hdev); if (status) return mgmt_cmd_status(sk, hdev->id, MGMT_OP_SET_HS, status); -- 2.7.4
On Tue, Feb 01, 2022 at 03:24:50PM +0100, Guillaume Bertholon wrote:
The upstream commit b560a208cda0 ("Bluetooth: MGMT: Fix not checking if BT_HS is enabled") inserted a new check in the `set_hs` function. However, its backported version in stable (commit 5abe9f99f512 ("Bluetooth: MGMT: Fix not checking if BT_HS is enabled")), added the check in `set_link_security` instead.
This patch restores the intent of the upstream commit by moving back the BT_HS check to `set_hs`.
Fixes: 5abe9f99f512 ("Bluetooth: MGMT: Fix not checking if BT_HS is enabled") Signed-off-by: Guillaume Bertholon guillaume.bertholon@ens.fr
net/bluetooth/mgmt.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Now queued up, thanks!
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Guillaume Bertholon