[bug report] __netif_receive_skb_core: pass skb by reference

[bug report] __netif_receive_skb_core: pass skb by reference

[Dan Carpenter]

Hello Boris Sukholitko,

The patch c0bbbdc32feb: "__netif_receive_skb_core: pass skb by
reference" from May 19, 2020, leads to the following static checker
warning:

	net/core/dev.c:5256 __netif_receive_skb_core()
	warn: 'skb' was already freed.

net/core/dev.c
  5230          }
  5231  
  5232          if (pt_prev) {
  5233                  if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))
  5234                          goto drop;
  5235                  *ppt_prev = pt_prev;
  5236          } else {
  5237  drop:
  5238                  if (!deliver_exact)
  5239                          atomic_long_inc(&skb->dev->rx_dropped);
  5240                  else
  5241                          atomic_long_inc(&skb->dev->rx_nohandler);
  5242                  kfree_skb(skb);
                                  ^^^

  5243                  /* Jamal, now you will not able to escape explaining
  5244                   * me how you were going to use this. :-)
  5245                   */
  5246                  ret = NET_RX_DROP;
  5247          }
  5248  
  5249  out:
  5250          /* The invariant here is that if *ppt_prev is not NULL
  5251           * then skb should also be non-NULL.
  5252           *
  5253           * Apparently *ppt_prev assignment above holds this invariant due to
  5254           * skb dereferencing near it.
  5255           */
  5256          *pskb = skb;
                ^^^^^^^^^^^^
Freed and also used in the caller.

  5257          return ret;
  5258  }

regards,
dan carpenter

Re: [bug report] __netif_receive_skb_core: pass skb by reference

[Boris Sukholitko]

Hi Dan,

On Fri, May 22, 2020 at 01:15:53PM +0300, Dan Carpenter wrote:
> Hello Boris Sukholitko,
> 
> The patch c0bbbdc32feb: "__netif_receive_skb_core: pass skb by
> reference" from May 19, 2020, leads to the following static checker
> warning:
> 
> 	net/core/dev.c:5256 __netif_receive_skb_core()
> 	warn: 'skb' was already freed.

IMHO, the static checker is wrong in this case. Please see below.

> 
> net/core/dev.c
>   5230          }
>   5231  
>   5232          if (pt_prev) {
>   5233                  if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))
>   5234                          goto drop;
>   5235                  *ppt_prev = pt_prev;
>   5236          } else {
>   5237  drop:
>   5238                  if (!deliver_exact)
>   5239                          atomic_long_inc(&skb->dev->rx_dropped);
>   5240                  else
>   5241                          atomic_long_inc(&skb->dev->rx_nohandler);
>   5242                  kfree_skb(skb);
>                                   ^^^

Notice how the *ppt_prev is *not* being set in the drop case, when we
are freeing the skb.

> 
>   5243                  /* Jamal, now you will not able to escape explaining
>   5244                   * me how you were going to use this. :-)
>   5245                   */
>   5246                  ret = NET_RX_DROP;
>   5247          }
>   5248  
>   5249  out:
>   5250          /* The invariant here is that if *ppt_prev is not NULL
>   5251           * then skb should also be non-NULL.
>   5252           *
>   5253           * Apparently *ppt_prev assignment above holds this invariant due to
>   5254           * skb dereferencing near it.
>   5255           */
>   5256          *pskb = skb;
>                 ^^^^^^^^^^^^
> Freed and also used in the caller.
> 

Although the skb may be freed at this point, the callers have their pt_prev
being set to NULL. Therefore none of them will try to use the bad skb.

Thanks,
Boris.

>   5257          return ret;
>   5258  }
> 
> regards,
> dan carpenter