Misplaced patch application? (stable/linux-4.4.y)

Misplaced patch application? (stable/linux-4.4.y)

[Guillaume Bertholon]

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

Re: Misplaced patch application? (stable/linux-4.4.y)

[Greg KH]

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

[PATCH stable 4.4] Bluetooth: MGMT: Fix misplaced BT_HS check

[Guillaume Bertholon]

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

Re: [PATCH stable 4.4] Bluetooth: MGMT: Fix misplaced BT_HS check

[Greg KH]

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