On Mon, Aug 2, 2021 at 8:19 AM Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>
> @@ -200,7 +211,7 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
> sk_for_each(sk, &hci_sk_list.head) {
> struct sk_buff *nskb;
>
> - if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
> + if (sk->sk_state != BT_BOUND || hci_hdev_from_sock(sk) != hdev)
> continue;
>
> /* Don't send frame to the socket it came from */
I'm not convinced this is a good idea.
Here we use hci_hdev_from_sock() outside the socket lock, so now that
HCI_UNREGISTER test is not very logical.
And it's positively stupid to start with a hdev, then walk the socket
list to look up another hdev, and then check "was it the original
hdev, but not unregistered"?
In other words - if you care about HCI_UNREGISTER state, it should be
*above* the whole loop. Not inside of it.
So I think this test should remain just that
hci_pi(sk)->hdev != hdev
and if HCI_UNREGISTER is relevant - and I don't think it is - it
should have been done earlier.
> @@ -536,8 +548,9 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
>
> hdr = skb_push(skb, HCI_MON_HDR_SIZE);
> hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN);
> - if (hci_pi(sk)->hdev)
> - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
> + hdev = hci_hdev_from_sock(sk);
> + if (!IS_ERR(hdev))
> + hdr->index = cpu_to_le16(hdev->id);
Same deal. The 'id' is valid even if it's unregistered.
So either it should have generated an error earlier, or we just shouldn't care.
The fact that the old code used to do HCI_DEV_NONE seems to be more
about the fact that the ID no longer existed. I think that if you
want to monitor a socket with a stale hdev, that's likely pointless
but valid.
So I think this should just keep using the hdev->id, even for a
HCI_UNREGISTER case.
That said, the *normal* case seems to be from the socket ioctl code,
so it's either explicitly not yet registered, or it just got
registered with a hdev, so I don't think it even matters. It looks
like the only case that HCI_UNREGISTER case would happen is likely the
magical replay case.
Which - again - I think would actually prefer the now still existing
hdev channel id.
> @@ -574,8 +588,9 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
>
> hdr = skb_push(skb, HCI_MON_HDR_SIZE);
> hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE);
> - if (hci_pi(sk)->hdev)
> - hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
> + hdev = hci_hdev_from_sock(sk);
> + if (!IS_ERR(hdev))
> + hdr->index = cpu_to_le16(hdev->id);
> else
> hdr->index = cpu_to_le16(HCI_DEV_NONE);
> hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
I think the monitor_ctrl close case is exactly the same case as the
open case above.
But the others look good to me.
So I *think* that the cases you actually want to catch are just the
ioctl/recvmsg/sendmsg ones. Not the special "monitor the hdev" ones.
That said, if you have some test-case that argues differently, then
hard data is obviously more valid than my blathering above.
Linus
