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

[Willem de Bruijn]

Willem de Bruijn wrote:
> 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@xxxxxxxxxx>
> > 
> > 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.

I was unable to find a path also with the help of Gemini.

It did spot an interesting case where a cloned unrefcounted zerocopy
skb can be created. But it is reduced to uncloned immediately.

skb_morph is like skb_clone (calls __skb_clone), but skips the
skb_orphan_frags check. Its only caller ensures that this is safe.
But I'm inclined to send a patch to net-next that makes skb_morph
itself safe, by orphaning as well (and updating the caller to
gracefully handle skb_morph failure).