Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

7 messages, 4 authors, 2019-03-04 · open the first message on its own page

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: David Ahern <hidden>
Date: 2019-03-02 02:27:24

On 2/28/19 10:57 AM, Peter Oskolkov wrote:
David: I'm not sure how to test GSO (I assume we are talking about GSO
here) in
the selftest: the encapping code sets SKB_GSO_DODGY flag, and veth does
not support
dodginess: "tx-gso-robust: off [fixed]".

If the "dodgy" flag is not set, then gso validation in dev.c passes, and
large GSO packets
happily go through; if the "dodgy" flag is set, "dodgy" GSO packets are
rejected, TCP does
segmentation, and non-GSO packets happily go through (with an mtu tweak
to the LWT tunnel).

So I see three options:
- add a sysctl to _not_ set SKB_GSO_DODGY flag in lwt_bpf.c =>
handle_gso_type();
- change veth to accept "dodgy" GSO packets
- test the code "as is", meaning that GSO will be tried and disabled by
TCP stack

Which approach would you prefer?
definitely not a sysctl.

After that, I don't have a suggestion for GSO at the moment.

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2019-03-04 02:55:21

On Fri, Mar 1, 2019 at 9:27 PM David Ahern [off-list ref] wrote:
On 2/28/19 10:57 AM, Peter Oskolkov wrote:
quoted
David: I'm not sure how to test GSO (I assume we are talking about GSO
here) in
the selftest: the encapping code sets SKB_GSO_DODGY flag, and veth does
not support
dodginess: "tx-gso-robust: off [fixed]".

If the "dodgy" flag is not set, then gso validation in dev.c passes, and
large GSO packets
happily go through; if the "dodgy" flag is set, "dodgy" GSO packets are
rejected, TCP does
segmentation, and non-GSO packets happily go through (with an mtu tweak
to the LWT tunnel).
Very few devices unconditionally accept dodgy packets (only veth?).

A device that lacks the robust gso feature will cause a gso packet
with dodgy flag to enter software gso instead of passing to device
segmentation offload.

That should be perfect for checking that the packets can be segmented
correctly with the new header.

If the gso layer drops the packets, that is not due to dropping all
dodgy sources. It will be dropped somewhere else inside gso,
indication that something is not as expected with the packet.
quoted
So I see three options:
- add a sysctl to _not_ set SKB_GSO_DODGY flag in lwt_bpf.c =>
handle_gso_type();
- change veth to accept "dodgy" GSO packets
Neither, as these would bypass segmentation offload and pass the large
packet to the receive path. It is more interesting to validate the
packet in gso.
quoted
- test the code "as is", meaning that GSO will be tried and disabled by
TCP stack

Which approach would you prefer?
definitely not a sysctl.

After that, I don't have a suggestion for GSO at the moment.

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2019-03-04 04:06:30

On Sun, Mar 3, 2019 at 9:55 PM Willem de Bruijn
[off-list ref] wrote:
On Fri, Mar 1, 2019 at 9:27 PM David Ahern [off-list ref] wrote:
quoted
On 2/28/19 10:57 AM, Peter Oskolkov wrote:
quoted
David: I'm not sure how to test GSO (I assume we are talking about GSO
here) in
the selftest: the encapping code sets SKB_GSO_DODGY flag, and veth does
not support
dodginess: "tx-gso-robust: off [fixed]".

If the "dodgy" flag is not set, then gso validation in dev.c passes, and
large GSO packets
happily go through; if the "dodgy" flag is set, "dodgy" GSO packets are
rejected, TCP does
segmentation, and non-GSO packets happily go through (with an mtu tweak
to the LWT tunnel).
Very few devices unconditionally accept dodgy packets (only veth?).
virtio-net, I meant. But there are a few other virtual devices, like
macvlan and xen-netfront
A device that lacks the robust gso feature will cause a gso packet
with dodgy flag to enter software gso instead of passing to device
segmentation offload.

That should be perfect for checking that the packets can be segmented
correctly with the new header.

If the gso layer drops the packets, that is not due to dropping all
dodgy sources. It will be dropped somewhere else inside gso,
indication that something is not as expected with the packet.
quoted
quoted
So I see three options:
- add a sysctl to _not_ set SKB_GSO_DODGY flag in lwt_bpf.c =>
handle_gso_type();
- change veth to accept "dodgy" GSO packets
Neither, as these would bypass segmentation offload and pass the large
packet to the receive path. It is more interesting to validate the
packet in gso.
That said, one solution would be to make veth gso_robust configurable
through ethtool, by advertising it in hw_features.


quoted
quoted
- test the code "as is", meaning that GSO will be tried and disabled by
TCP stack

Which approach would you prefer?
definitely not a sysctl.

After that, I don't have a suggestion for GSO at the moment.

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: Peter Oskolkov <hidden>
Date: 2019-03-04 20:39:38

On Sun, Mar 3, 2019 at 6:55 PM Willem de Bruijn
[off-list ref] wrote:
On Fri, Mar 1, 2019 at 9:27 PM David Ahern [off-list ref] wrote:
quoted
On 2/28/19 10:57 AM, Peter Oskolkov wrote:
quoted
David: I'm not sure how to test GSO (I assume we are talking about GSO
here) in
the selftest: the encapping code sets SKB_GSO_DODGY flag, and veth does
not support
dodginess: "tx-gso-robust: off [fixed]".

If the "dodgy" flag is not set, then gso validation in dev.c passes, and
large GSO packets
happily go through; if the "dodgy" flag is set, "dodgy" GSO packets are
rejected, TCP does
segmentation, and non-GSO packets happily go through (with an mtu tweak
to the LWT tunnel).
Very few devices unconditionally accept dodgy packets (only veth?).

A device that lacks the robust gso feature will cause a gso packet
with dodgy flag to enter software gso instead of passing to device
segmentation offload.

That should be perfect for checking that the packets can be segmented
correctly with the new header.

If the gso layer drops the packets, that is not due to dropping all
dodgy sources. It will be dropped somewhere else inside gso,
indication that something is not as expected with the packet.
quoted
quoted
So I see three options:
- add a sysctl to _not_ set SKB_GSO_DODGY flag in lwt_bpf.c =>
handle_gso_type();
- change veth to accept "dodgy" GSO packets
Neither, as these would bypass segmentation offload and pass the large
packet to the receive path. It is more interesting to validate the
packet in gso.
I found the problem: skb->inner_protocol was not set, so software GSO
fallback failed.  I have a patch that fixes the issue: IPIP+GRE+TCP
gso works! net-next is closed though... Will have to wait for net-next
to reopen.

quoted
quoted
- test the code "as is", meaning that GSO will be tried and disabled by
TCP stack

Which approach would you prefer?
definitely not a sysctl.

After that, I don't have a suggestion for GSO at the moment.

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: David Ahern <hidden>
Date: 2019-03-04 21:03:25

On 3/4/19 1:39 PM, Peter Oskolkov wrote:
I found the problem: skb->inner_protocol was not set, so software GSO
fallback failed.  I have a patch that fixes the issue: IPIP+GRE+TCP
gso works! net-next is closed though... Will have to wait for net-next
to reopen.
That's a bug fix. I suggest sending now.

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: Peter Oskolkov <hidden>
Date: 2019-03-04 22:37:58

On Mon, Mar 4, 2019 at 1:03 PM David Ahern [off-list ref] wrote:
On 3/4/19 1:39 PM, Peter Oskolkov wrote:
quoted
I found the problem: skb->inner_protocol was not set, so software GSO
fallback failed.  I have a patch that fixes the issue: IPIP+GRE+TCP
gso works! net-next is closed though... Will have to wait for net-next
to reopen.
That's a bug fix. I suggest sending now.
I see the encap patches neither in net nor in bpf trees, only in
net-next and bpf-next. And *-next trees are closed, so there is
nowhere to send the fix. Am I missing something?

Re: [PATCH bpf-next] bpf: fix memory leak in bpf_lwt_xmit_reroute

From: Eric Dumazet <hidden>
Date: 2019-03-04 23:28:34


On 03/04/2019 02:37 PM, Peter Oskolkov wrote:
On Mon, Mar 4, 2019 at 1:03 PM David Ahern [off-list ref] wrote:
quoted
On 3/4/19 1:39 PM, Peter Oskolkov wrote:
quoted
I found the problem: skb->inner_protocol was not set, so software GSO
fallback failed.  I have a patch that fixes the issue: IPIP+GRE+TCP
gso works! net-next is closed though... Will have to wait for net-next
to reopen.
That's a bug fix. I suggest sending now.
I see the encap patches neither in net nor in bpf trees, only in
net-next and bpf-next. And *-next trees are closed, so there is
nowhere to send the fix. Am I missing something?
David has not yet sent his pull request to Linus.

This means your fix needs to target net-next

net-next is closed for patches that are meant for linux-5.2,
but 'open' for fixes (targeting 5.1)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help