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

[Geliang Tang <geliangtang@gmail.com>]

Hi Paolo,

Thank you so much for this patch. It fixed the issue that bothered me
for a long time. With this change, it seems the bad data in the
MP_FAIL multiple subflows test case has been dropped correctly now:

> sudo ./mptcp_join.sh -F
Created /tmp/tmp.lqIytszwoM (size 1 KB) containing data sent by client
Created /tmp/tmp.zChji3JlEJ (size 1 KB) containing data sent by server
Created /tmp/tmp.iTuFZCucnH (size 128 KB) containing data sent by client
Created /tmp/tmp.YTN6JELVhq (size 128 KB) containing data sent by server
file received by server has inverted byte at 169
001 Infinite map: 5 corrupted pkts       syn[ ok ] - synack[ ok ] - ack[ ok ]
                                                            sum[ ok ]
- csum  [ ok ]
                                                            ftx[ ok ]
- failrx[ ok ]
                                                            rtx[ ok ]
- rstrx [ ok ]
                                                            itx[ ok ]
- infirx[ ok ]
                                                            ftx[ ok ]
- failrx[ ok ] invert
Created /tmp/tmp.iTuFZCucnH (size 1024 KB) containing data sent by client
Created /tmp/tmp.YTN6JELVhq (size 1024 KB) containing data sent by server
002 MP_FAIL MP_RST: 1 corrupted pkts     syn[ ok ] - synack[ ok ] - ack[ ok ]

sum[ ok ] - csum  [ ok ]

ftx[ ok ] - failrx[ ok ]

rtx[ ok ] - rstrx [ ok ]

itx[ ok ] - infirx[ ok ]

No inverted byte is received in the MP_FAIL MP_RST test.

Do I understand this correctly?

Anyway, please add my tested-by tag for this patch:

Tested-by: Geliang Tang <geliang.tang@suse.com>

Thanks,
-Geliang

Paolo Abeni <pabeni@redhat.com> 于2022年5月2日周一 15:55写道：

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