* [PATCH] Bluetooth: Simplify / fix return values from tk_request
@ 2020-04-03 15:02 Guenter Roeck
2020-04-03 15:13 ` Alain Michaud
2020-04-06 19:15 ` Sonny Sasaka
0 siblings, 2 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-04-03 15:02 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hedberg, David S . Miller, Jakub Kicinski, linux-bluetooth,
netdev, linux-kernel, Guenter Roeck, Sonny Sasaka
Some static checker run by 0day reports a variableScope warning.
net/bluetooth/smp.c:870:6: warning:
The scope of the variable 'err' can be reduced. [variableScope]
There is no need for two separate variables holding return values.
Stick with the existing variable. While at it, don't pre-initialize
'ret' because it is set in each code path.
tk_request() is supposed to return a negative error code on errors,
not a bluetooth return code. The calling code converts the return
value to SMP_UNSPECIFIED if needed.
Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
Cc: Sonny Sasaka <sonnysasaka@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
net/bluetooth/smp.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index d0b695ee49f6..30e8626dd553 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
struct l2cap_chan *chan = conn->smp;
struct smp_chan *smp = chan->data;
u32 passkey = 0;
- int ret = 0;
- int err;
+ int ret;
/* Initialize key for JUST WORKS */
memset(smp->tk, 0, sizeof(smp->tk));
@@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
/* If Just Works, Continue with Zero TK and ask user-space for
* confirmation */
if (smp->method == JUST_WORKS) {
- err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
+ ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
hcon->type,
hcon->dst_type,
passkey, 1);
- if (err)
- return SMP_UNSPECIFIED;
+ if (ret)
+ return ret;
set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-03 15:02 [PATCH] Bluetooth: Simplify / fix return values from tk_request Guenter Roeck
@ 2020-04-03 15:13 ` Alain Michaud
2020-04-03 16:42 ` Guenter Roeck
2020-04-06 19:15 ` Sonny Sasaka
1 sibling, 1 reply; 10+ messages in thread
From: Alain Michaud @ 2020-04-03 15:13 UTC (permalink / raw)
To: Guenter Roeck
Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski,
BlueZ, netdev, LKML, Sonny Sasaka
Hi Guenter/Marcel,
On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Some static checker run by 0day reports a variableScope warning.
>
> net/bluetooth/smp.c:870:6: warning:
> The scope of the variable 'err' can be reduced. [variableScope]
>
> There is no need for two separate variables holding return values.
> Stick with the existing variable. While at it, don't pre-initialize
> 'ret' because it is set in each code path.
>
> tk_request() is supposed to return a negative error code on errors,
> not a bluetooth return code. The calling code converts the return
> value to SMP_UNSPECIFIED if needed.
>
> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> net/bluetooth/smp.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index d0b695ee49f6..30e8626dd553 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> struct l2cap_chan *chan = conn->smp;
> struct smp_chan *smp = chan->data;
> u32 passkey = 0;
> - int ret = 0;
> - int err;
> + int ret;
>
> /* Initialize key for JUST WORKS */
> memset(smp->tk, 0, sizeof(smp->tk));
> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> /* If Just Works, Continue with Zero TK and ask user-space for
> * confirmation */
> if (smp->method == JUST_WORKS) {
> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> hcon->type,
> hcon->dst_type,
> passkey, 1);
> - if (err)
> - return SMP_UNSPECIFIED;
> + if (ret)
> + return ret;
I think there may be some miss match between expected types of error
codes here. The SMP error code type seems to be expected throughout
this code base, so this change would propagate a potential negative
value while the rest of the SMP protocol expects strictly positive
error codes.
> set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
> return 0;
> }
> --
> 2.17.1
>
Thanks,
Alain
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-03 15:13 ` Alain Michaud
@ 2020-04-03 16:42 ` Guenter Roeck
2020-04-03 16:56 ` Alain Michaud
2020-04-06 12:06 ` Marcel Holtmann
0 siblings, 2 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-04-03 16:42 UTC (permalink / raw)
To: Alain Michaud
Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski,
BlueZ, netdev, LKML, Sonny Sasaka
On 4/3/20 8:13 AM, Alain Michaud wrote:
> Hi Guenter/Marcel,
>
>
> On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Some static checker run by 0day reports a variableScope warning.
>>
>> net/bluetooth/smp.c:870:6: warning:
>> The scope of the variable 'err' can be reduced. [variableScope]
>>
>> There is no need for two separate variables holding return values.
>> Stick with the existing variable. While at it, don't pre-initialize
>> 'ret' because it is set in each code path.
>>
>> tk_request() is supposed to return a negative error code on errors,
>> not a bluetooth return code. The calling code converts the return
>> value to SMP_UNSPECIFIED if needed.
>>
>> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
>> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> net/bluetooth/smp.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
>> index d0b695ee49f6..30e8626dd553 100644
>> --- a/net/bluetooth/smp.c
>> +++ b/net/bluetooth/smp.c
>> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
>> struct l2cap_chan *chan = conn->smp;
>> struct smp_chan *smp = chan->data;
>> u32 passkey = 0;
>> - int ret = 0;
>> - int err;
>> + int ret;
>>
>> /* Initialize key for JUST WORKS */
>> memset(smp->tk, 0, sizeof(smp->tk));
>> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
>> /* If Just Works, Continue with Zero TK and ask user-space for
>> * confirmation */
>> if (smp->method == JUST_WORKS) {
>> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
>> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
>> hcon->type,
>> hcon->dst_type,
>> passkey, 1);
>> - if (err)
>> - return SMP_UNSPECIFIED;
>> + if (ret)
>> + return ret;
> I think there may be some miss match between expected types of error
> codes here. The SMP error code type seems to be expected throughout
> this code base, so this change would propagate a potential negative
> value while the rest of the SMP protocol expects strictly positive
> error codes.
>
Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request()
returned negative error codes, and all callers convert it to SMP_UNSPECIFIED.
If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should
be returned consistently, and its callers don't have to convert it again.
Guenter
>> set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
>> return 0;
>> }
>> --
>> 2.17.1
>>
>
> Thanks,
> Alain
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-03 16:42 ` Guenter Roeck
@ 2020-04-03 16:56 ` Alain Michaud
2020-04-04 0:39 ` Sonny Sasaka
2020-04-06 12:06 ` Marcel Holtmann
1 sibling, 1 reply; 10+ messages in thread
From: Alain Michaud @ 2020-04-03 16:56 UTC (permalink / raw)
To: Guenter Roeck
Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski,
BlueZ, netdev, LKML, Sonny Sasaka
On Fri, Apr 3, 2020 at 12:43 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 4/3/20 8:13 AM, Alain Michaud wrote:
> > Hi Guenter/Marcel,
> >
> >
> > On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> Some static checker run by 0day reports a variableScope warning.
> >>
> >> net/bluetooth/smp.c:870:6: warning:
> >> The scope of the variable 'err' can be reduced. [variableScope]
> >>
> >> There is no need for two separate variables holding return values.
> >> Stick with the existing variable. While at it, don't pre-initialize
> >> 'ret' because it is set in each code path.
> >>
> >> tk_request() is supposed to return a negative error code on errors,
> >> not a bluetooth return code. The calling code converts the return
> >> value to SMP_UNSPECIFIED if needed.
> >>
> >> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
> >> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> net/bluetooth/smp.c | 9 ++++-----
> >> 1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> >> index d0b695ee49f6..30e8626dd553 100644
> >> --- a/net/bluetooth/smp.c
> >> +++ b/net/bluetooth/smp.c
> >> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> >> struct l2cap_chan *chan = conn->smp;
> >> struct smp_chan *smp = chan->data;
> >> u32 passkey = 0;
> >> - int ret = 0;
> >> - int err;
> >> + int ret;
> >>
> >> /* Initialize key for JUST WORKS */
> >> memset(smp->tk, 0, sizeof(smp->tk));
> >> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> >> /* If Just Works, Continue with Zero TK and ask user-space for
> >> * confirmation */
> >> if (smp->method == JUST_WORKS) {
> >> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> >> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> >> hcon->type,
> >> hcon->dst_type,
> >> passkey, 1);
> >> - if (err)
> >> - return SMP_UNSPECIFIED;
> >> + if (ret)
> >> + return ret;
> > I think there may be some miss match between expected types of error
> > codes here. The SMP error code type seems to be expected throughout
> > this code base, so this change would propagate a potential negative
> > value while the rest of the SMP protocol expects strictly positive
> > error codes.
> >
>
> Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request()
> returned negative error codes, and all callers convert it to SMP_UNSPECIFIED.
>
> If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should
> be returned consistently, and its callers don't have to convert it again.
Agreed, the conventions aren't clear here. I'll differ to Marcel to
provide guidance in this case where as a long term solution might
increase the scope of this patch beyond what would be reasonable.
>
> Guenter
>
> >> set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
> >> return 0;
> >> }
> >> --
> >> 2.17.1
> >>
> >
> > Thanks,
> > Alain
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-03 16:56 ` Alain Michaud
@ 2020-04-04 0:39 ` Sonny Sasaka
0 siblings, 0 replies; 10+ messages in thread
From: Sonny Sasaka @ 2020-04-04 0:39 UTC (permalink / raw)
To: Alain Michaud
Cc: Guenter Roeck, Marcel Holtmann, Johan Hedberg, David S . Miller,
Jakub Kicinski, BlueZ, netdev, LKML
The patch looks good to me. Agreed with Guenter's assessment, I made a
mistake in the original patch by not being consistent with the
function contract.
On Fri, Apr 3, 2020 at 9:57 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> On Fri, Apr 3, 2020 at 12:43 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 4/3/20 8:13 AM, Alain Michaud wrote:
> > > Hi Guenter/Marcel,
> > >
> > >
> > > On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > >>
> > >> Some static checker run by 0day reports a variableScope warning.
> > >>
> > >> net/bluetooth/smp.c:870:6: warning:
> > >> The scope of the variable 'err' can be reduced. [variableScope]
> > >>
> > >> There is no need for two separate variables holding return values.
> > >> Stick with the existing variable. While at it, don't pre-initialize
> > >> 'ret' because it is set in each code path.
> > >>
> > >> tk_request() is supposed to return a negative error code on errors,
> > >> not a bluetooth return code. The calling code converts the return
> > >> value to SMP_UNSPECIFIED if needed.
> > >>
> > >> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
> > >> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
> > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > >> ---
> > >> net/bluetooth/smp.c | 9 ++++-----
> > >> 1 file changed, 4 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> > >> index d0b695ee49f6..30e8626dd553 100644
> > >> --- a/net/bluetooth/smp.c
> > >> +++ b/net/bluetooth/smp.c
> > >> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> > >> struct l2cap_chan *chan = conn->smp;
> > >> struct smp_chan *smp = chan->data;
> > >> u32 passkey = 0;
> > >> - int ret = 0;
> > >> - int err;
> > >> + int ret;
> > >>
> > >> /* Initialize key for JUST WORKS */
> > >> memset(smp->tk, 0, sizeof(smp->tk));
> > >> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> > >> /* If Just Works, Continue with Zero TK and ask user-space for
> > >> * confirmation */
> > >> if (smp->method == JUST_WORKS) {
> > >> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> > >> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> > >> hcon->type,
> > >> hcon->dst_type,
> > >> passkey, 1);
> > >> - if (err)
> > >> - return SMP_UNSPECIFIED;
> > >> + if (ret)
> > >> + return ret;
> > > I think there may be some miss match between expected types of error
> > > codes here. The SMP error code type seems to be expected throughout
> > > this code base, so this change would propagate a potential negative
> > > value while the rest of the SMP protocol expects strictly positive
> > > error codes.
> > >
> >
> > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request()
> > returned negative error codes, and all callers convert it to SMP_UNSPECIFIED.
> >
> > If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should
> > be returned consistently, and its callers don't have to convert it again.
> Agreed, the conventions aren't clear here. I'll differ to Marcel to
> provide guidance in this case where as a long term solution might
> increase the scope of this patch beyond what would be reasonable.
> >
> > Guenter
> >
> > >> set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
> > >> return 0;
> > >> }
> > >> --
> > >> 2.17.1
> > >>
> > >
> > > Thanks,
> > > Alain
> > >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-03 16:42 ` Guenter Roeck
2020-04-03 16:56 ` Alain Michaud
@ 2020-04-06 12:06 ` Marcel Holtmann
2020-04-06 18:13 ` Sonny Sasaka
1 sibling, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2020-04-06 12:06 UTC (permalink / raw)
To: Guenter Roeck
Cc: Alain Michaud, Johan Hedberg, David S. Miller, Jakub Kicinski,
BlueZ, netdev, LKML, Sonny Sasaka
Hi Guenter,
>>> Some static checker run by 0day reports a variableScope warning.
>>>
>>> net/bluetooth/smp.c:870:6: warning:
>>> The scope of the variable 'err' can be reduced. [variableScope]
>>>
>>> There is no need for two separate variables holding return values.
>>> Stick with the existing variable. While at it, don't pre-initialize
>>> 'ret' because it is set in each code path.
>>>
>>> tk_request() is supposed to return a negative error code on errors,
>>> not a bluetooth return code. The calling code converts the return
>>> value to SMP_UNSPECIFIED if needed.
>>>
>>> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
>>> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> net/bluetooth/smp.c | 9 ++++-----
>>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
>>> index d0b695ee49f6..30e8626dd553 100644
>>> --- a/net/bluetooth/smp.c
>>> +++ b/net/bluetooth/smp.c
>>> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
>>> struct l2cap_chan *chan = conn->smp;
>>> struct smp_chan *smp = chan->data;
>>> u32 passkey = 0;
>>> - int ret = 0;
>>> - int err;
>>> + int ret;
>>>
>>> /* Initialize key for JUST WORKS */
>>> memset(smp->tk, 0, sizeof(smp->tk));
>>> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
>>> /* If Just Works, Continue with Zero TK and ask user-space for
>>> * confirmation */
>>> if (smp->method == JUST_WORKS) {
>>> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
>>> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
>>> hcon->type,
>>> hcon->dst_type,
>>> passkey, 1);
>>> - if (err)
>>> - return SMP_UNSPECIFIED;
>>> + if (ret)
>>> + return ret;
>> I think there may be some miss match between expected types of error
>> codes here. The SMP error code type seems to be expected throughout
>> this code base, so this change would propagate a potential negative
>> value while the rest of the SMP protocol expects strictly positive
>> error codes.
>>
>
> Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request()
> returned negative error codes, and all callers convert it to SMP_UNSPECIFIED.
>
> If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should
> be returned consistently, and its callers don't have to convert it again.
maybe we need to fix that initial patch then.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-06 12:06 ` Marcel Holtmann
@ 2020-04-06 18:13 ` Sonny Sasaka
2020-04-06 18:26 ` Marcel Holtmann
0 siblings, 1 reply; 10+ messages in thread
From: Sonny Sasaka @ 2020-04-06 18:13 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Guenter Roeck, Alain Michaud, Johan Hedberg, David S. Miller,
Jakub Kicinski, BlueZ, netdev, LKML
Hi Marcel,
Can this patch be merged? Or do you prefer reverting the original
patch and relanding it together with the fix?
On Mon, Apr 6, 2020 at 5:06 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Guenter,
>
> >>> Some static checker run by 0day reports a variableScope warning.
> >>>
> >>> net/bluetooth/smp.c:870:6: warning:
> >>> The scope of the variable 'err' can be reduced. [variableScope]
> >>>
> >>> There is no need for two separate variables holding return values.
> >>> Stick with the existing variable. While at it, don't pre-initialize
> >>> 'ret' because it is set in each code path.
> >>>
> >>> tk_request() is supposed to return a negative error code on errors,
> >>> not a bluetooth return code. The calling code converts the return
> >>> value to SMP_UNSPECIFIED if needed.
> >>>
> >>> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
> >>> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >>> ---
> >>> net/bluetooth/smp.c | 9 ++++-----
> >>> 1 file changed, 4 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> >>> index d0b695ee49f6..30e8626dd553 100644
> >>> --- a/net/bluetooth/smp.c
> >>> +++ b/net/bluetooth/smp.c
> >>> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> >>> struct l2cap_chan *chan = conn->smp;
> >>> struct smp_chan *smp = chan->data;
> >>> u32 passkey = 0;
> >>> - int ret = 0;
> >>> - int err;
> >>> + int ret;
> >>>
> >>> /* Initialize key for JUST WORKS */
> >>> memset(smp->tk, 0, sizeof(smp->tk));
> >>> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> >>> /* If Just Works, Continue with Zero TK and ask user-space for
> >>> * confirmation */
> >>> if (smp->method == JUST_WORKS) {
> >>> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> >>> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> >>> hcon->type,
> >>> hcon->dst_type,
> >>> passkey, 1);
> >>> - if (err)
> >>> - return SMP_UNSPECIFIED;
> >>> + if (ret)
> >>> + return ret;
> >> I think there may be some miss match between expected types of error
> >> codes here. The SMP error code type seems to be expected throughout
> >> this code base, so this change would propagate a potential negative
> >> value while the rest of the SMP protocol expects strictly positive
> >> error codes.
> >>
> >
> > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request()
> > returned negative error codes, and all callers convert it to SMP_UNSPECIFIED.
> >
> > If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should
> > be returned consistently, and its callers don't have to convert it again.
>
> maybe we need to fix that initial patch then.
>
> Regards
>
> Marcel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-06 18:13 ` Sonny Sasaka
@ 2020-04-06 18:26 ` Marcel Holtmann
2020-04-06 18:45 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2020-04-06 18:26 UTC (permalink / raw)
To: Sonny Sasaka
Cc: Guenter Roeck, Alain Michaud, Johan Hedberg, David S. Miller,
Jakub Kicinski, BlueZ, netdev, LKML
Hi Sonny,
> Can this patch be merged? Or do you prefer reverting the original
> patch and relanding it together with the fix?
since the original patch is already upstream, I need a patch with Fixes etc. And a Reviewed-By from you preferably.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-06 18:26 ` Marcel Holtmann
@ 2020-04-06 18:45 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-04-06 18:45 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Sonny Sasaka, Alain Michaud, Johan Hedberg, David S. Miller,
Jakub Kicinski, BlueZ, netdev, LKML
On Mon, Apr 06, 2020 at 08:26:55PM +0200, Marcel Holtmann wrote:
> Hi Sonny,
>
> > Can this patch be merged? Or do you prefer reverting the original
> > patch and relanding it together with the fix?
>
> since the original patch is already upstream, I need a patch with Fixes etc. And a Reviewed-By from you preferably.
>
Isn't that what I sent with https://patchwork.kernel.org/patch/11472991/ ?
What is missing (besides Sonny's Reviewed-by: tag) ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request
2020-04-03 15:02 [PATCH] Bluetooth: Simplify / fix return values from tk_request Guenter Roeck
2020-04-03 15:13 ` Alain Michaud
@ 2020-04-06 19:15 ` Sonny Sasaka
1 sibling, 0 replies; 10+ messages in thread
From: Sonny Sasaka @ 2020-04-06 19:15 UTC (permalink / raw)
To: Guenter Roeck
Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski,
BlueZ, netdev, LKML
Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
On Fri, Apr 3, 2020 at 8:02 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Some static checker run by 0day reports a variableScope warning.
>
> net/bluetooth/smp.c:870:6: warning:
> The scope of the variable 'err' can be reduced. [variableScope]
>
> There is no need for two separate variables holding return values.
> Stick with the existing variable. While at it, don't pre-initialize
> 'ret' because it is set in each code path.
>
> tk_request() is supposed to return a negative error code on errors,
> not a bluetooth return code. The calling code converts the return
> value to SMP_UNSPECIFIED if needed.
>
> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
> Cc: Sonny Sasaka <sonnysasaka@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> net/bluetooth/smp.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index d0b695ee49f6..30e8626dd553 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> struct l2cap_chan *chan = conn->smp;
> struct smp_chan *smp = chan->data;
> u32 passkey = 0;
> - int ret = 0;
> - int err;
> + int ret;
>
> /* Initialize key for JUST WORKS */
> memset(smp->tk, 0, sizeof(smp->tk));
> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
> /* If Just Works, Continue with Zero TK and ask user-space for
> * confirmation */
> if (smp->method == JUST_WORKS) {
> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
> hcon->type,
> hcon->dst_type,
> passkey, 1);
> - if (err)
> - return SMP_UNSPECIFIED;
> + if (ret)
> + return ret;
> set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
> return 0;
> }
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-06 19:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-03 15:02 [PATCH] Bluetooth: Simplify / fix return values from tk_request Guenter Roeck
2020-04-03 15:13 ` Alain Michaud
2020-04-03 16:42 ` Guenter Roeck
2020-04-03 16:56 ` Alain Michaud
2020-04-04 0:39 ` Sonny Sasaka
2020-04-06 12:06 ` Marcel Holtmann
2020-04-06 18:13 ` Sonny Sasaka
2020-04-06 18:26 ` Marcel Holtmann
2020-04-06 18:45 ` Guenter Roeck
2020-04-06 19:15 ` Sonny Sasaka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).