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

[Greg KH <gregkh@linuxfoundation.org>]

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