Re: [PATCH] net: add missing skb_uarg refcount increment in pskb_carve_inside_header()

Re: [PATCH] net: add missing skb_uarg refcount increment in pskb_carve_inside_header()

[linmiaohe]

Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>On Fri, Aug 14, 2020 at 9:20 AM linmiaohe <linmiaohe@huawei.com> wrote:
>>
>> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>> >On Thu, Aug 13, 2020 at 2:16 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>> >>
>> >> If the skb is zcopied, we should increase the skb_uarg refcount 
>> >> before we involve skb_release_data(). See pskb_expand_head() as a reference.
>> >
>> >Did you manage to observe a bug through this datapath in practice?
>> >
>> >pskb_carve_inside_header is called
>> >  from pskb_carve
>> >    from pskb_extract
>> >      from rds_tcp_data_recv
>> >
>> >That receive path should not see any packets with zerocopy state associated.
>> >
>>
>> This works fine yet as its caller is limited. But we should take care of the skb_uarg refcount for future use.
>
>If a new application of this interface is proposed, the author will have to make sure that it is exercised correctly.

Sure. Let the author make sure that it is exercised correctly if a new application of this interface is proposed.

>> On the other hand, because this codepath should not see any packets 
>> with zerocopy state associated, then we should not call skb_orphan_frags here.

>I'm also not convinced that the skb_orphan_frags here are needed, given the only path is from tcp_read_sock.

Maybe just keep it here as it doesn't hurt even if it's really not needed.

Many thanks.

Re: [PATCH] net: add missing skb_uarg refcount increment in pskb_carve_inside_header()

[Willem de Bruijn]

On Fri, Aug 14, 2020 at 9:20 AM linmiaohe <linmiaohe@huawei.com> wrote:
>
> Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> >On Thu, Aug 13, 2020 at 2:16 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
> >>
> >> If the skb is zcopied, we should increase the skb_uarg refcount before
> >> we involve skb_release_data(). See pskb_expand_head() as a reference.
> >
> >Did you manage to observe a bug through this datapath in practice?
> >
> >pskb_carve_inside_header is called
> >  from pskb_carve
> >    from pskb_extract
> >      from rds_tcp_data_recv
> >
> >That receive path should not see any packets with zerocopy state associated.
> >
>
> This works fine yet as its caller is limited. But we should take care of the skb_uarg refcount for future use.

If a new application of this interface is proposed, the author will
have to make sure that it is exercised correctly.

> On the other hand, because this codepath should not see any packets with zerocopy state associated, then we
> should not call skb_orphan_frags here.

I'm also not convinced that the skb_orphan_frags here are needed,
given the only path is from tcp_read_sock.

Re: [PATCH] net: add missing skb_uarg refcount increment in pskb_carve_inside_header()

[linmiaohe]

Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>On Thu, Aug 13, 2020 at 2:16 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> If the skb is zcopied, we should increase the skb_uarg refcount before 
>> we involve skb_release_data(). See pskb_expand_head() as a reference.
>
>Did you manage to observe a bug through this datapath in practice?
>
>pskb_carve_inside_header is called
>  from pskb_carve
>    from pskb_extract
>      from rds_tcp_data_recv
>
>That receive path should not see any packets with zerocopy state associated.
>

This works fine yet as its caller is limited. But we should take care of the skb_uarg refcount for future use.
On the other hand, because this codepath should not see any packets with zerocopy state associated, then we
should not call skb_orphan_frags here.

Thanks.

>> Fixes: 6fa01ccd8830 ("skbuff: Add pskb_extract() helper function")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Re: [PATCH] net: add missing skb_uarg refcount increment in pskb_carve_inside_header()

[Willem de Bruijn]

On Thu, Aug 13, 2020 at 2:16 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> If the skb is zcopied, we should increase the skb_uarg refcount before we
> involve skb_release_data(). See pskb_expand_head() as a reference.

Did you manage to observe a bug through this datapath in practice?

pskb_carve_inside_header is called
  from pskb_carve
    from pskb_extract
      from rds_tcp_data_recv

That receive path should not see any packets with zerocopy state associated.


> Fixes: 6fa01ccd8830 ("skbuff: Add pskb_extract() helper function")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

[PATCH] net: add missing skb_uarg refcount increment in pskb_carve_inside_header()

[Miaohe Lin]

If the skb is zcopied, we should increase the skb_uarg refcount before we
involve skb_release_data(). See pskb_expand_head() as a reference.

Fixes: 6fa01ccd8830 ("skbuff: Add pskb_extract() helper function")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 net/core/skbuff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 475f9aa51b57..975600558e8b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5842,6 +5842,8 @@ static int pskb_carve_inside_header(struct sk_buff *skb, const u32 off,
 			kfree(data);
 			return -ENOMEM;
 		}
+		if (skb_zcopy(skb))
+			refcount_inc(&skb_uarg(skb)->refcnt);
 		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
 			skb_frag_ref(skb, i);
 		if (skb_has_frag_list(skb))
-- 
2.19.1