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:
> > 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;