Re: Possible unsafe usage of skb->cb in virtio-net

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 02, 2017 at 11:40:36AM +0000, Ilya Lesokhin wrote:
> Hi,
> I've noticed that the virtio-net uses skb->cb.
> 
> I don't know all the detail by my understanding is it caused problem with the mlx5 driver
> and was fixed here:
> https://github.com/torvalds/linux/commit/34802a42b3528b0e18ea4517c8b23e1214a09332
> 
> Thanks,
> Ilya

Thanks a lot for the pointer.

I think this was in response to this:
https://patchwork.ozlabs.org/patch/558324/

> >
> > +     skb_push(skb, skb->data - skb_data_orig);
> >       sq->skb[pi] = skb;
> >
> >       MLX5E_TX_SKB_CB(skb)->num_wqebbs = DIV_ROUND_UP(ds_cnt,
> 
> And in the middle of this we have:
> 
>                 skb_pull_inline(skb, ihs);
> 
> This is looks illegal.
> 
> You must not modify the data pointers of any SKB that you receive for
> sending via ->ndo_start_xmit() unless you know that absolutely you are
> the one and only reference that exists to that SKB.
> 
> And exactly for the case you are trying to "fix" here, you do not.  If
> the SKB is cloned, or has an elevated users count, someone else can be
> looking at it exactly at the same time you are messing with the data
> pointers.
> 
> I bet mlx4 has this bug too.
> 
> You must fix this properly, by keeping track of an offset or similar
> internally to your driver, rather than changing the SKB data pointers.

What virtio does is this:

        can_push = vi->any_header_sg &&
                !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
                !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
        /* Even if we can, don't push here yet as this would skew
         * csum_start offset below. */
        if (can_push)
                hdr = (struct virtio_net_hdr_mrg_rxbuf *)(skb->data - hdr_len);
        else
                hdr = skb_vnet_hdr(skb);


This doesn't change the data pointers in a cloned skb but it does change the cb.
Is it true that it's illegal to touch the cb in a cloned skb then?

-- 
MST