* Re: Possible unsafe usage of skb->cb in virtio-net
[not found] <VI1PR0501MB2734AAE4311FE84881F622A2D45C0@VI1PR0501MB2734.eurprd05.prod.outlook.com>
2017-11-02 13:01 ` Possible unsafe usage of skb->cb in virtio-net Michael S. Tsirkin
@ 2017-11-02 13:01 ` Michael S. Tsirkin
2017-11-03 4:57 ` Willem de Bruijn
` (2 more replies)
1 sibling, 3 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-11-02 13:01 UTC (permalink / raw)
To: Ilya Lesokhin
Cc: virtualization, netdev, David Miller, Willem de Bruijn, Jason Wang
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible unsafe usage of skb->cb in virtio-net
[not found] <VI1PR0501MB2734AAE4311FE84881F622A2D45C0@VI1PR0501MB2734.eurprd05.prod.outlook.com>
@ 2017-11-02 13:01 ` Michael S. Tsirkin
2017-11-02 13:01 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-11-02 13:01 UTC (permalink / raw)
To: Ilya Lesokhin; +Cc: netdev, Willem de Bruijn, David Miller, virtualization
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible unsafe usage of skb->cb in virtio-net
2017-11-02 13:01 ` Michael S. Tsirkin
2017-11-03 4:57 ` Willem de Bruijn
@ 2017-11-03 4:57 ` Willem de Bruijn
2017-12-19 9:06 ` Jason Wang
2 siblings, 0 replies; 5+ messages in thread
From: Willem de Bruijn @ 2017-11-03 4:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Ilya Lesokhin, Network Development, Willem de Bruijn,
David Miller, virtualization
On Thu, Nov 2, 2017 at 10:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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?
I don't have all the context for this bug. But in general, clones do not share
the struct sk_buff, which holds the CB. So skb_push and skb_pull_inline
cannot affect the view of other clones. If an skb is shared, that's a different
story.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible unsafe usage of skb->cb in virtio-net
2017-11-02 13:01 ` Michael S. Tsirkin
@ 2017-11-03 4:57 ` Willem de Bruijn
2017-11-03 4:57 ` Willem de Bruijn
2017-12-19 9:06 ` Jason Wang
2 siblings, 0 replies; 5+ messages in thread
From: Willem de Bruijn @ 2017-11-03 4:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, Willem de Bruijn, David Miller, virtualization
On Thu, Nov 2, 2017 at 10:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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?
I don't have all the context for this bug. But in general, clones do not share
the struct sk_buff, which holds the CB. So skb_push and skb_pull_inline
cannot affect the view of other clones. If an skb is shared, that's a different
story.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible unsafe usage of skb->cb in virtio-net
2017-11-02 13:01 ` Michael S. Tsirkin
2017-11-03 4:57 ` Willem de Bruijn
2017-11-03 4:57 ` Willem de Bruijn
@ 2017-12-19 9:06 ` Jason Wang
2 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2017-12-19 9:06 UTC (permalink / raw)
To: Michael S. Tsirkin, Ilya Lesokhin
Cc: netdev, Willem de Bruijn, David Miller, virtualization
On 2017年11月02日 21:01, Michael S. Tsirkin wrote:
> 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?
>
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-19 9:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <VI1PR0501MB2734AAE4311FE84881F622A2D45C0@VI1PR0501MB2734.eurprd05.prod.outlook.com>
2017-11-02 13:01 ` Possible unsafe usage of skb->cb in virtio-net Michael S. Tsirkin
2017-11-02 13:01 ` Michael S. Tsirkin
2017-11-03 4:57 ` Willem de Bruijn
2017-11-03 4:57 ` Willem de Bruijn
2017-12-19 9:06 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.