Re: [PATCH mptcp-net v2] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni <pabeni@redhat.com>]

On Fri, 2022-04-29 at 13:56 -0700, Mat Martineau wrote:
> On Fri, 29 Apr 2022, Paolo Abeni wrote:
> 
> > Currently pedit tries to ensure that the accessed skb offset
> > is writeble via skb_unclone(). The action potentially allows
> > touching any skb bytes, so it may end-up modifying shared data.
> > 
> > The above causes some sporadic MPTCP self-test failures.
> > 
> > Address the issue keeping track of a rough over-estimate highest skb
> > offset accessed by the action and ensure such offset is really
> > writable.
> > 
> > Note that this may cause performance regressions in some scenario,
> > but hopefully pedit is not critical path.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v1 -> v2:
> > - fix build issue
> > - account for the skb hdr offset, too
> > 
> > this almost solves issues/265 here. I'm still getting some rare
> > failure with MPTcpExtMPFailTx==0: sometimes the transfer completes
> > before we are able to use the 2nd/failing link. The relevant fix
> > is a purely seft-test one
> > 
> > Note that a much simpler alternatives would be simply replacing
> > skb_unshare() with skb_ensure_writable(skb, skb->len), but that
> > really could causes more visible regressions
> 
> To make sure I'm understanding correctly: skb_ensure_writable(skb, 
> skb->len) would copy the entire packet payload on every edited 
> packet, but this patch will only copy the part that might be modified (and 
> maybe a little extra). 
> 
Yes, that is. All the above when the relevant packet is cloned.

> Seems like the full copy is worth avoiding, and 
> that users shouldn't be depending on pedit modifying shared data.
> 
> I did run the associated test for a while (with the other patches for 
> #265) and the changes look good from a MPTCP perspective:
> 
> Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> Do you plan to upstream this one yourself, or should I include it with the 
> other mptcp-net patches?

I think it can (and should, to avoid blocking mptcp patches with
net/sched discussion and vice-versa) go on it's own. I can send it.

Thanks!

Paolo