Hello Luiz Augusto von Dentz,
The patch eca0ae4aea66: "Bluetooth: Add initial implementation of BIS
connections" from Mar 9, 2022, leads to the following Smatch static
checker warning:
net/bluetooth/hci_conn.c:943 hci_le_remove_cig()
warn: passing zero to 'ERR_PTR'
net/bluetooth/hci_conn.c
939 static int hci_le_remove_cig(struct hci_dev *hdev, u8 handle)
^^^^^^^^^^
940 {
941 bt_dev_dbg(hdev, "handle 0x%2.2x", handle);
942
--> 943 return hci_cmd_sync_queue(hdev, remove_cig_sync, ERR_PTR(handle), NULL);
^^^^^^
handle is a u8. It can hold error codes.
944 }
regards,
dan carpenter
On Wed, May 24, 2023 at 09:38:34AM +0300, Dan Carpenter wrote:
> Hello Luiz Augusto von Dentz,
>
> The patch eca0ae4aea66: "Bluetooth: Add initial implementation of BIS
> connections" from Mar 9, 2022, leads to the following Smatch static
> checker warning:
>
> net/bluetooth/hci_conn.c:943 hci_le_remove_cig()
> warn: passing zero to 'ERR_PTR'
>
> net/bluetooth/hci_conn.c
> 939 static int hci_le_remove_cig(struct hci_dev *hdev, u8 handle)
> ^^^^^^^^^^
>
> 940 {
> 941 bt_dev_dbg(hdev, "handle 0x%2.2x", handle);
> 942
> --> 943 return hci_cmd_sync_queue(hdev, remove_cig_sync, ERR_PTR(handle), NULL);
> ^^^^^^
> handle is a u8. It can hold error codes.
s/can/cannot/.
regards,
dan carpenter
Hi Dan,
On Tue, May 23, 2023 at 11:52 PM Dan Carpenter <[email protected]> wrote:
>
> On Wed, May 24, 2023 at 09:38:34AM +0300, Dan Carpenter wrote:
> > Hello Luiz Augusto von Dentz,
> >
> > The patch eca0ae4aea66: "Bluetooth: Add initial implementation of BIS
> > connections" from Mar 9, 2022, leads to the following Smatch static
> > checker warning:
> >
> > net/bluetooth/hci_conn.c:943 hci_le_remove_cig()
> > warn: passing zero to 'ERR_PTR'
> >
> > net/bluetooth/hci_conn.c
> > 939 static int hci_le_remove_cig(struct hci_dev *hdev, u8 handle)
> > ^^^^^^^^^^
> >
> > 940 {
> > 941 bt_dev_dbg(hdev, "handle 0x%2.2x", handle);
> > 942
> > --> 943 return hci_cmd_sync_queue(hdev, remove_cig_sync, ERR_PTR(handle), NULL);
> > ^^^^^^
> > handle is a u8. It can hold error codes.
>
> s/can/cannot/.
This is not used as an error though, it is more like INT_PTR, that
said I'm fine changing it if that is triggering static analyzer
errors.
> regards,
> dan carpenter
>
--
Luiz Augusto von Dentz
On Wed, May 24, 2023 at 12:11:08PM -0700, Luiz Augusto von Dentz wrote:
> > > net/bluetooth/hci_conn.c:943 hci_le_remove_cig()
> > > warn: passing zero to 'ERR_PTR'
> > >
> > > net/bluetooth/hci_conn.c
> > > 939 static int hci_le_remove_cig(struct hci_dev *hdev, u8 handle)
> > > ^^^^^^^^^^
> > >
> > > 940 {
> > > 941 bt_dev_dbg(hdev, "handle 0x%2.2x", handle);
> > > 942
> > > --> 943 return hci_cmd_sync_queue(hdev, remove_cig_sync, ERR_PTR(handle), NULL);
> > > ^^^^^^
> > > handle is a u8. It can hold error codes.
> >
> > s/can/cannot/.
>
> This is not used as an error though, it is more like INT_PTR, that
> said I'm fine changing it if that is triggering static analyzer
> errors.
Yes, please. There is quite a bit of static analysis to ensure that
ERR_PTR() values are actually errors. For example, people pass the
results from copy_to_user(), or the return from true/false functions or
they accidentally return a positive ERR_PTR(EINVAL); All those lead to
a crash in the caller.
I'm sort of surprised we don't already have an INT_PTR() macro...
regards,
dan carpenter