netdev - Re: [PATCH v6 net-next 1/2] net: Prevent RPS table overwrite for active flows

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: <CACLgkEY4cRWsRQW=-PSxnE=V6AvRuKuvYzXSuofmB8NMJ=9ZqQ@mail.gmail.com>
Date: Mon, 28 Jul 2025 07:43:25 +0530
From: Krishna Kumar <krikku@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
	tom@...bertland.com, pabeni@...hat.com, horms@...nel.org, sdf@...ichev.me, 
	kuniyu@...gle.com, ahmed.zaki@...el.com, aleksander.lobakin@...el.com, 
	atenart@...nel.org, krishna.ku@...pkart.com
Subject: Re: [PATCH v6 net-next 1/2] net: Prevent RPS table overwrite for
 active flows

On Sat, Jul 26, 2025 at 4:15 AM Jakub Kicinski <kuba@...nel.org> wrote:

> > +     return cpu < nr_cpu_ids &&
> > +            ((int)(READ_ONCE(per_cpu(softnet_data, cpu).input_queue_head) -
> > +             READ_ONCE(rflow->last_qtail)) < (int)(10 << flow_table->log));
>
> Since you're factoring this condition out you could split it up a bit.
> Save the result of at least that READ_ONCE() that takes up an entire
> line to a temporary variable?

I agree that will make the line far easier to understand.

> > +             /* Make sure this slot is usable before enabling steer */
>
> This comment is less clear than the code..

Ack. Will remove.

> > +                             /*
> > +                              * This slot has an active "programmed" flow.
> > +                              * Break out if the cached value stored is for
> > +                              * a different flow, or (for our flow) the
> > +                              * rx-queue# did not change.
> > +                              */
>
> This just restates what the code does

Ack.

> > +                             if (hash != READ_ONCE(tmp_rflow->hash) ||
> > +                                 next_cpu == tmp_cpu) {
> > +                                     /*
> > +                                      * Don't unnecessarily reprogram if:
> > +                                      * 1. This slot has an active different
> > +                                      *    flow.
> > +                                      * 2. This slot has the same flow (very
> > +                                      *    likely but not guaranteed) and
> > +                                      *    the rx-queue# did not change.
> > +                                      */

I took some time to figure out the different paths here as it was a
new area for me, hence I put this comment. Shall I keep it as the
condition is not very intuitive?

> > +                     /*
> > +                      * When we overwrite the flow, the driver still has
> > +                      * the cached entry. But drivers will check if the
> > +                      * flow is active and rps_may_expire_entry() will
> > +                      * return False and driver will delete it soon. Hence
> > +                      * inconsistency between kernel & driver is quickly
> > +                      * resolved.
> > +                      */
>
> I don't get this comment either, and rps_may_expire_entry() does not
> exist in my tree.

Apologies for the typo (rps_may_expire_flow). This comment can be removed
as it is already known to driver writers on the consistency of rules between the
kernel and the driver (the driver caches its version of the flow
internally before
programming the rule). I will remove this.

Thanks,
- Krishna