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

On 2017年11月02日 21:01, Michael S. Tsirkin wrote:

On Thu, Nov 02, 2017 at 11:40:36AM +0000, Ilya Lesokhin wrote:

quoted

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/

quoted

quoted

+     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?

I think not.

skb_clone() call __skb_copy_header() which did:

     memcpy(new->cb, old->cb, sizeof(old->cb));

Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization