Re: [PATCH net] net: skbuff: fix missing zerocopy reference in pskb_carve helpers

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Willem de Bruijn wrote:
> Willem de Bruijn wrote:
> > Willem de Bruijn wrote:
> > > lazyming wrote:
> > > > pskb_carve_inside_header() and pskb_carve_inside_nonlinear() both copy
> > > > the old skb_shared_info header into a new buffer via memcpy(), which
> > > > includes the destructor_arg pointer (uarg) for MSG_ZEROCOPY skbs.
> > > 
> > > These functions are not supposed to maintain zerocopy frags.
> > > 
> > > Both call skb_orphan_frags.
> > > 
> > > I think what may need to happen is to invert the order of that call
> > > and the memcpy. Current code:
> > > 
> > >         memcpy((struct skb_shared_info *)(data + size),
> > >                skb_shinfo(skb), offsetof(struct skb_shared_info, frags[0]));
> > >         if (skb_orphan_frags(skb, gfp_mask)) {
> > >                 skb_kfree_head(data);
> > >                 return -ENOMEM;
> > >         }
> > 
> > Never mind. This actually corresponds to the first Sashiko report you
> > mentioned: if zerocopy skbs are converted, then the memcpy prior to
> > that call will have stale state.
> > 
> > For skbs where skb_orphan_frags does not do a deep copy, we do need to
> > take this extra reference.
> > 
> > Reviewed-by: Willem de Bruijn <willemb@google.com>
> 
> Not sure the potential preexisting issue is reachable.
> 
> Vhost-net and other zerocopy that predates MSG_ZEROCOPY does not
> refcount ubuf_info. Instead it calls skb_copy_ubufs on skb_clone.
> 
> So if such an skb reaches pskb_expand_head, it should be guaranteed to
> not be a clone. Same for the carve methods added later.
> 
> But, the commit that added zerocopy, commit a6686f2f382b
> ("skbuff: skb supports zero-copy buffers"), included this 
> pksb_expand_head call to skb_copy_ubufs from the start. That implies
> that was expected to be reachable. I just don't see how yet.
> 
> If it is reachable, then all that is needed is to clear shinfo->flags.
> Or more neatly,
> 
>     skb_shinfo(skb)->flags &= ~SKBFL_ALL_ZEROCOPY;

Also, I'm not the expert on more recent managed frags
(SKBFL_MANAGED_FRAG_REFS).

That calls skb_zcopy_downgrade_managed in pskb_expand_head, but not in
the two other functions with memcpy before skb_copy_ubufs:
pskb_carve_inside_header and pskb_carve_inside_nonlinear.

I assume because those shorten the skb, so no risk of getting mixed
mode refcounted and non-refcounted frags?

In general zerocopy can be split in refcounted and non-refcounted.

Refcounted zerocopy will not downgrade in these cases, so will not
modify shinfo->flags after memcpy.

Non-refcounted should always get converted to copy in skb_clone,
so will not enter the skb_cloned() branch here.

If in doubt maybe warrants a rare WARN_ON_ONCE patch.