[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f2f90f1f-f231-42ad-b94e-6960ef248a20@stanley.mountain>
Date: Fri, 16 Aug 2024 21:00:38 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Rao Shoaib <rao.shoaib@...cle.com>
Cc: netdev@...r.kernel.org
Subject: Re: [bug report] af_unix: Add OOB support
On Fri, Aug 16, 2024 at 10:28:14AM -0700, Rao Shoaib wrote:
>
>
> On 8/16/24 10:10, Dan Carpenter wrote:
> > On Fri, Aug 16, 2024 at 09:50:56AM -0700, Rao Shoaib wrote:
> >>
> >>
> >> On 8/16/24 07:22, Dan Carpenter wrote:
> >>> Hello Rao Shoaib,
> >>>
> >>> Commit 314001f0bf92 ("af_unix: Add OOB support") from Aug 1, 2021
> >>> (linux-next), leads to the following Smatch static checker warning:
> >>>
> >>> net/unix/af_unix.c:2718 manage_oob()
> >>> warn: 'skb' was already freed. (line 2699)
> >>>
> >>> net/unix/af_unix.c
> >>> 2665 static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
> >>> 2666 int flags, int copied)
> >>> 2667 {
> >>> 2668 struct unix_sock *u = unix_sk(sk);
> >>> 2669
> >>> 2670 if (!unix_skb_len(skb)) {
> >>> 2671 struct sk_buff *unlinked_skb = NULL;
> >>> 2672
> >>> 2673 spin_lock(&sk->sk_receive_queue.lock);
> >>> 2674
> >>> 2675 if (copied && (!u->oob_skb || skb == u->oob_skb)) {
> >>> 2676 skb = NULL;
> >>> 2677 } else if (flags & MSG_PEEK) {
> >>> 2678 skb = skb_peek_next(skb, &sk->sk_receive_queue);
> >>> 2679 } else {
> >>> 2680 unlinked_skb = skb;
> >>> 2681 skb = skb_peek_next(skb, &sk->sk_receive_queue);
> >>> 2682 __skb_unlink(unlinked_skb, &sk->sk_receive_queue);
> >>> 2683 }
> >>> 2684
> >>> 2685 spin_unlock(&sk->sk_receive_queue.lock);
> >>> 2686
> >>> 2687 consume_skb(unlinked_skb);
> >>> 2688 } else {
> >>> 2689 struct sk_buff *unlinked_skb = NULL;
> >>> 2690
> >>> 2691 spin_lock(&sk->sk_receive_queue.lock);
> >>> 2692
> >>> 2693 if (skb == u->oob_skb) {
> >>> 2694 if (copied) {
> >>> 2695 skb = NULL;
> >>> 2696 } else if (!(flags & MSG_PEEK)) {
> >>> 2697 if (sock_flag(sk, SOCK_URGINLINE)) {
> >>> 2698 WRITE_ONCE(u->oob_skb, NULL);
> >>> 2699 consume_skb(skb);
> >>>
> >>> Why are we returning this freed skb? It feels like we should return NULL.
> >>
> >> Hi Dan,
> >>
> >> manage_oob is called from the following code segment
> >>
> >> #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> >> if (skb) {
> >> skb = manage_oob(skb, sk, flags, copied);
> >> if (!skb && copied) {
> >> unix_state_unlock(sk);
> >> break;
> >> }
> >> }
> >> #endif
> >>
> >> So skb can not be NULL when manage_oob is called. The code that you
> >> pointed out may free the skb (if the refcnts were incorrect) but skb
> >> would not be NULL. It seems to me that the checker is incorrect or maybe
> >> there is a way that skb maybe NULL and I am just not seeing it.
> >>
> >> If you can explain to me how skb can be NULL, I will be happy to fix the
> >> issue.
> >>
> >
> > No, I was suggesting maybe we *should* return NULL. The question is why are we
> > returning a freed skb pointer?
> >
> > regards,
> > dan carpenter
>
> We are not returning a freed skb pointer. The refcnt's protect the skb
> from being freed. Now if somehow the refcnts are wrong and the skb gets
> freed, that is a different issue and is a bug.
>
Ah ok. Thanks!
regards,
dan carpenter
Powered by blists - more mailing lists