netdev - Re: [PATCH bpf-next v2 18/20] selftests/bpf: Use AF_INET for TX in xdp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 7 Jul 2023 09:49:51 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, 
	andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com, 
	john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com, 
	jolsa@...nel.org, David Ahern <dsahern@...il.com>, Jakub Kicinski <kuba@...nel.org>, 
	Willem de Bruijn <willemb@...gle.com>, Jesper Dangaard Brouer <brouer@...hat.com>, 
	Anatoly Burakov <anatoly.burakov@...el.com>, Alexander Lobakin <alexandr.lobakin@...el.com>, 
	Magnus Karlsson <magnus.karlsson@...il.com>, Maryam Tahhan <mtahhan@...hat.com>, 
	xdp-hints@...-project.net, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 18/20] selftests/bpf: Use AF_INET for TX in xdp_metadata

On 07/07, Larysa Zaremba wrote:
> On Thu, Jul 06, 2023 at 10:27:38AM -0700, Stanislav Fomichev wrote:
> > On Thu, Jul 6, 2023 at 7:15 AM Larysa Zaremba <larysa.zaremba@...el.com> wrote:
> > >
> > > On Wed, Jul 05, 2023 at 10:39:35AM -0700, Stanislav Fomichev wrote:
> > > > On 07/03, Larysa Zaremba wrote:
> > > > > The easiest way to simulate stripped VLAN tag in veth is to send a packet
> > > > > from VLAN interface, attached to veth. Unfortunately, this approach is
> > > > > incompatible with AF_XDP on TX side, because VLAN interfaces do not have
> > > > > such feature.
> > > > >
> > > > > Replace AF_XDP packet generation with sending the same datagram via
> > > > > AF_INET socket.
> > > > >
> > > > > This does not change the packet contents or hints values with one notable
> > > > > exception: rx_hash_type, which previously was expected to be 0, now is
> > > > > expected be at least XDP_RSS_TYPE_L4.
> > > > >
> > > > > Also, usage of AF_INET requires a little more complicated namespace setup,
> > > > > therefore open_netns() helper function is divided into smaller reusable
> > > > > pieces.
> > > >
> > > > Ack, it's probably OK for now, but, FYI, I'm trying to extend this part
> > > > with TX metadata:
> > > > https://lore.kernel.org/bpf/20230621170244.1283336-10-sdf@google.com/
> > > >
> > > > So probably long-term I'll switch it back to AF_XDP but will add
> > > > support for requesting vlan TX "offload" from the veth.
> > > >
> > >
> > > My bad for not reading your series. Amazing work as always!
> > >
> > > So, 'requesting vlan TX "offload"' with new hints capabilities? This would be
> > > pretty neat.
> > >
> > > But you think AF_INET TX is worth keeping for now, until TX hints are mature?
> > >
> > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> > > > > ---
> > > > >  tools/testing/selftests/bpf/network_helpers.c |  37 +++-
> > > > >  tools/testing/selftests/bpf/network_helpers.h |   3 +
> > > > >  .../selftests/bpf/prog_tests/xdp_metadata.c   | 175 +++++++-----------
> > > > >  3 files changed, 98 insertions(+), 117 deletions(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> > > > > index a105c0cd008a..19463230ece5 100644
> > > > > --- a/tools/testing/selftests/bpf/network_helpers.c
> > > > > +++ b/tools/testing/selftests/bpf/network_helpers.c
> > > > > @@ -386,28 +386,51 @@ char *ping_command(int family)
> > > > >     return "ping";
> > > > >  }
> > > > >
> > > > > +int get_cur_netns(void)
> > > > > +{
> > > > > +   int nsfd;
> > > > > +
> > > > > +   nsfd = open("/proc/self/ns/net", O_RDONLY);
> > > > > +   ASSERT_GE(nsfd, 0, "open /proc/self/ns/net");
> > > > > +   return nsfd;
> > > > > +}
> > > > > +
> > > > > +int get_netns(const char *name)
> > > > > +{
> > > > > +   char nspath[PATH_MAX];
> > > > > +   int nsfd;
> > > > > +
> > > > > +   snprintf(nspath, sizeof(nspath), "%s/%s", "/var/run/netns", name);
> > > > > +   nsfd = open(nspath, O_RDONLY | O_CLOEXEC);
> > > > > +   ASSERT_GE(nsfd, 0, "open /proc/self/ns/net");
> > > > > +   return nsfd;
> > > > > +}
> > > > > +
> > > > > +int set_netns(int netns_fd)
> > > > > +{
> > > > > +   return setns(netns_fd, CLONE_NEWNET);
> > > > > +}
> > > >
> > > > We have open_netns/close_netns in network_helpers.h that provide similar
> > > > functionality, let's use them instead?
> > > >
> > >
> > > I have divided open_netns() into smaller pieces (see below), because the code I
> > > have added into xdp_metadata looked better with those smaller pieces (I had to
> > > switch namespace several times).
> > 
> > Forgot to reply to this part. I missed the fact that you're extending
> > network_helpers, sorry.
> > But why do we need extra namespaces at all?
> 
> If veths are in the same namespace, AF_INET packets are not sent between them,
> so XDP is skipped. So we need 2 test namespaces: for RX and TX.

Makes sense. But let's maybe use the existing helpers to jump to/from
namespaces?

It might be a bit more verbose, but it makes it easy to annotate namespace
being/end. (compared to random jumping around with setns)

tok = open_netns("tx");
do_something();
close_netns(tok);

tok = open_netns("rx");
do_something_else();
close_netns(tok);

Should be doable?