Hi Dan,
On Wed, Sep 15, 2021 at 2:27 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Luiz Augusto von Dentz,
>
> The patch 81be03e026dc: "Bluetooth: RFCOMM: Replace use of
> memcpy_from_msg with bt_skb_sendmmsg" from Sep 3, 2021, leads to the
> following
> Smatch static checker warning:
>
> net/bluetooth/rfcomm/sock.c:587 rfcomm_sock_sendmsg()
> warn: passing zero to 'PTR_ERR'
>
> net/bluetooth/rfcomm/sock.c
> 556 static int rfcomm_sock_sendmsg(struct socket *sock, struct msghdr *msg,
> 557 size_t len)
> 558 {
> 559 struct sock *sk = sock->sk;
> 560 struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
> 561 struct sk_buff *skb;
> 562 int sent;
> 563
> 564 if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
> 565 return -ENOTCONN;
> 566
> 567 if (msg->msg_flags & MSG_OOB)
> 568 return -EOPNOTSUPP;
> 569
> 570 if (sk->sk_shutdown & SEND_SHUTDOWN)
> 571 return -EPIPE;
> 572
> 573 BT_DBG("sock %p, sk %p", sock, sk);
> 574
> 575 lock_sock(sk);
> 576
> 577 sent = bt_sock_wait_ready(sk, msg->msg_flags);
> 578
> 579 release_sock(sk);
> 580
> 581 if (sent)
> 582 return sent;
> 583
> 584 skb = bt_skb_sendmmsg(sk, msg, len, d->mtu, RFCOMM_SKB_HEAD_RESERVE,
> 585 RFCOMM_SKB_TAIL_RESERVE);
> 586 if (IS_ERR_OR_NULL(skb))
>
> When a function returns both error pointers and NULL then that means
> the feature is optional and can be turned off by the user.
>
> blinking_lights = get_blinking_lights();
>
> We should report the error to the user.
>
> if (IS_ERR(blinking_lights))
> return PTR_ERR(blinking_lights);
>
> However, some users maybe want a smaller kernel with no blinking lights
> so they disable it. In that case the driver has to check for NULL, and
> not print an error message but instead continue as best as possible
> without that feature enabled.
>
> The bt_skb_sendmmsg() cannot return NULL. But if it did return NULL
> then PTR_ERR(NULL) is success so that's not right... All the callers
> of bt_skb_sendmmsg() have the same issue.
It should never be NULL though:
skb = bt_skb_send_alloc(sk, size + headroom + tailroom,
msg->msg_flags & MSG_DONTWAIT, &err);
if (!skb)
return ERR_PTR(err);
So I guess using IS_ERR_OR_NULL is misleading since we use the err
that comes from sock_alloc_send_skb so we might as well replace that
with IS_ERR instead.
> --> 587 return PTR_ERR(skb);
> 588
> 589 sent = rfcomm_dlc_send(d, skb);
> 590 if (sent < 0)
> 591 kfree_skb(skb);
> 592
> 593 return sent;
> 594 }
>
> regards,
> dan carpenter
--
Luiz Augusto von Dentz
