From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: Possible unsafe usage of skb->cb in virtio-net Date: Thu, 2 Nov 2017 15:01:11 +0200 Message-ID: <20171102142758-mutt-send-email-mst@kernel.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, David Miller , Willem de Bruijn , Jason Wang To: Ilya Lesokhin Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39644 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752392AbdKBNBN (ORCPT ); Thu, 2 Nov 2017 09:01:13 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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