[bug report] net: dsa: ksz: fix skb freeing

[bug report] net: dsa: ksz: fix skb freeing

[Dan Carpenter]

Hello Vivien Didelot,

The patch e71cb9e00922: "net: dsa: ksz: fix skb freeing" from Aug 9,
2017, leads to the following static checker warning:

	net/dsa/tag_ksz.c:64 ksz_xmit()
	warn: 'nskb' was already freed.

net/dsa/tag_ksz.c
    35  static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
    36  {
    37          struct dsa_slave_priv *p = netdev_priv(dev);
    38          struct sk_buff *nskb;
    39          int padlen;
    40          u8 *tag;
    41  
    42          padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len;
    43  
    44          if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) {
    45                  if (skb_put_padto(skb, skb->len + padlen))
    46                          return NULL;
    47  
    48                  nskb = skb;
    49          } else {
    50                  nskb = alloc_skb(NET_IP_ALIGN + skb->len +
    51                                   padlen + KSZ_INGRESS_TAG_LEN, GFP_ATOMIC);
    52                  if (!nskb)
    53                          return NULL;
    54                  skb_reserve(nskb, NET_IP_ALIGN);
    55  
    56                  skb_reset_mac_header(nskb);
    57                  skb_set_network_header(nskb,
    58                                         skb_network_header(skb) - skb->head);
    59                  skb_set_transport_header(nskb,
    60                                           skb_transport_header(skb) - skb->head);
    61                  skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len));
    62  
    63                  if (skb_put_padto(nskb, nskb->len + padlen)) {
    64                          kfree_skb(nskb);
                                ^^^^^^^^^^^^^^
This patch deliberately adds a kfree_skb() here, but it looks like a
double free, and my static checker complains.  My guess is you had
intended to free "skb" instead of "nskb"?  I'm not positive.

    65                          return NULL;
    66                  }
    67  
    68                  kfree_skb(skb);
    69          }
    70  
    71          tag = skb_put(nskb, KSZ_INGRESS_TAG_LEN);
    72          tag[0] = 0;
    73          tag[1] = 1 << p->dp->index; /* destination port */
    74  
    75          return nskb;
    76  }

regards,
dan carpenter

Re: [bug report] net: dsa: ksz: fix skb freeing

[Andrew Lunn]

On Thu, Aug 17, 2017 at 12:27:19AM +0300, Dan Carpenter wrote:
> Hello Vivien Didelot,
> 
> The patch e71cb9e00922: "net: dsa: ksz: fix skb freeing" from Aug 9,
> 2017, leads to the following static checker warning:
> 
> 	net/dsa/tag_ksz.c:64 ksz_xmit()
> 	warn: 'nskb' was already freed.

> net/dsa/tag_ksz.c
>     35  static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
>     36  {
>     37          struct dsa_slave_priv *p = netdev_priv(dev);
>     38          struct sk_buff *nskb;
>     39          int padlen;
>     40          u8 *tag;
>     41  
>     42          padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len;
>     43  
>     44          if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) {
>     45                  if (skb_put_padto(skb, skb->len + padlen))
>     46                          return NULL;
>     47  
>     48                  nskb = skb;
>     49          } else {
>     50                  nskb = alloc_skb(NET_IP_ALIGN + skb->len +
>     51                                   padlen + KSZ_INGRESS_TAG_LEN, GFP_ATOMIC);
>     52                  if (!nskb)
>     53                          return NULL;
>     54                  skb_reserve(nskb, NET_IP_ALIGN);
>     55  
>     56                  skb_reset_mac_header(nskb);
>     57                  skb_set_network_header(nskb,
>     58                                         skb_network_header(skb) - skb->head);
>     59                  skb_set_transport_header(nskb,
>     60                                           skb_transport_header(skb) - skb->head);
>     61                  skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len));
>     62  
>     63                  if (skb_put_padto(nskb, nskb->len + padlen)) {
>     64                          kfree_skb(nskb);
>                                 ^^^^^^^^^^^^^^
> This patch deliberately adds a kfree_skb() here, but it looks like a
> double free, and my static checker complains.  My guess is you had
> intended to free "skb" instead of "nskb"?  I'm not positive.

I actually think it is a miss understanding about skb_put_padto()
actually does. If it returns an error code, it also frees the skb it
has been asked to modify. So free'ing nsbk is wrong. So line 64 simply
needs to be removed.

      Andrew