| Message ID | 20200403150236.74232-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | Bluetooth: Simplify / fix return values from tk_request | expand |
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
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 >
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 > > >
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 > > > > >
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
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 >
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
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
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 >
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; }
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(-)