netdev - Re: [PATCH net-next v3 2/2] selftests/bpf: check if the global consumer of tx queue updates after send call

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aFwPCsSFkLYYoFu9@mini-arch>
Date: Wed, 25 Jun 2025 08:00:26 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: Maciej Fijalkowski <maciej.fijalkowski@...el.com>, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	bjorn@...nel.org, magnus.karlsson@...el.com,
	jonathan.lemon@...il.com, sdf@...ichev.me, ast@...nel.org,
	daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com,
	joe@...a.to, willemdebruijn.kernel@...il.com, bpf@...r.kernel.org,
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 2/2] selftests/bpf: check if the global
 consumer of tx queue updates after send call

On 06/25, Jason Xing wrote:
> On Wed, Jun 25, 2025 at 8:19 PM Maciej Fijalkowski
> <maciej.fijalkowski@...el.com> wrote:
> >
> > On Wed, Jun 25, 2025 at 06:10:14PM +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@...cent.com>
> > >
> > > The subtest sends 33 packets at one time on purpose to see if xsk
> > > exitting __xsk_generic_xmit() updates the global consumer of tx queue
> > > when reaching the max loop (max_tx_budget, 32 by default). The number 33
> > > can avoid xskq_cons_peek_desc() updates the consumer, to accurately
> > > check if the issue that the first patch resolves remains.
> > >
> > > Speaking of the selftest implementation, it's not possible to use the
> > > normal validation_func to check if the issue happens because the whole
> > > send packets logic will call the sendto multiple times such that we're
> > > unable to detect in time.
> > >
> > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > ---
> > >  tools/testing/selftests/bpf/xskxceiver.c | 30 ++++++++++++++++++++++--
> > >  1 file changed, 28 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > > index 0ced4026ee44..f7aa83706bc7 100644
> > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > @@ -109,6 +109,8 @@
> > >
> > >  #include <network_helpers.h>
> > >
> > > +#define MAX_TX_BUDGET_DEFAULT 32
> >
> > and what if in the future you would increase the generic xmit budget on
> > the system? it would be better to wait with test addition when you
> > introduce the setsockopt patch.

We can always update it to follow new budget. The purpose of the test
is to document/verify userspace expectations. Sincle even with the
setsockopt we are still gonna have the default budget.

> > plus keep in mind that xskxceiver tests ZC drivers as well. so either we
> > should have a test that serves all modes or keep it for skb mode only.
> >
> > > +
> > >  static bool opt_verbose;
> > >  static bool opt_print_tests;
> > >  static enum test_mode opt_mode = TEST_MODE_ALL;
> > > @@ -1323,7 +1325,8 @@ static int receive_pkts(struct test_spec *test)
> > >       return TEST_PASS;
> > >  }
> > >
> > > -static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, bool timeout)
> > > +static int __send_pkts(struct test_spec *test, struct ifobject *ifobject,
> > > +                    struct xsk_socket_info *xsk, bool timeout)
> > >  {
> > >       u32 i, idx = 0, valid_pkts = 0, valid_frags = 0, buffer_len;
> > >       struct pkt_stream *pkt_stream = xsk->pkt_stream;
> > > @@ -1437,9 +1440,21 @@ static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, b
> > >       }
> > >
> > >       if (!timeout) {
> > > +             int prev_tx_consumer;
> > > +
> > > +             if (!strncmp("TX_QUEUE_CONSUMER", test->name, MAX_TEST_NAME_SIZE))
> > > +                     prev_tx_consumer = *xsk->tx.consumer;
> > > +
> > >               if (complete_pkts(xsk, i))
> > >                       return TEST_FAILURE;
> > >
> > > +             if (!strncmp("TX_QUEUE_CONSUMER", test->name, MAX_TEST_NAME_SIZE)) {
> > > +                     int delta = *xsk->tx.consumer - prev_tx_consumer;
> >
> > hacking the data path logic for single test purpose is rather not good.
> > I am also not really sure if this deserves a standalone test case or could
> > we just introduce a check in data path in appropriate place.
> 
> The big headache is that if we expect to detect such a case, we have
> to re-invent a similar send packet logic or hack the data path (a bit
> like this patch). I admit it's ugly as I mentioned yesterday.
> 
> Sorry, Stanislav, no offense here. If you read this, please don't
> blame me. I know you wish me to add one related test case. So here we
> are. Since Maciej brought up the similar thought, I keep wondering if
> we should give up such a standalone test patch? Honestly it already
> involved more time than expected. The primary reason for me is that
> the issue doesn't cause much trouble to the application.

IIUC, Maciej does not suggest to completely drop the test but rather
to move this check (unconditionally and only for skb mode) somewhere
into __send_pkts/complete_pkts to make sure the number of completed
packets is always <= budget. Maciej correct me if I misread..