netdev - Re: [PATCH v5 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: <CACLgkEbBFCoUBMu5x3Gezx8nihSnn3BB9cH51T3LahVii1FspQ@mail.gmail.com>
Date: Mon, 21 Jul 2025 17:03:38 +0530
From: Krishna Kumar <krikku@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, tom@...bertland.com, 
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, sdf@...ichev.me, 
	kuniyu@...gle.com, ahmed.zaki@...el.com, aleksander.lobakin@...el.com, 
	atenart@...nel.org, jdamato@...tly.com, krishna.ku@...pkart.com
Subject: Re: [PATCH v5 net-next 1/2] net: Prevent RPS table overwrite for
 active flows

On Mon, Jul 21, 2025 at 1:51 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> > +static bool rps_flow_is_active(struct rps_dev_flow *rflow,
> > +                              struct rps_dev_flow_table *flow_table,
> > +                              unsigned int cpu)
> > +{
> > +       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));
> > +}
> > +#endif
>
> This notion of active flow is kind of weird.
> It might be time to make it less obscure, less expensive and time
> (jiffies ?) deterministic.

My first internal attempt had this approach (not submitted as I felt
it was doing two
things in the same patch - fixing an issue we are seeing in Flipkart
production servers
vs improving an existing function):

struct rps_dev_flow {
        u16 cpu;
        u16 filter;
        unsigned int last_qtail;
        unsigned long last_active; /* Last activity timestamp (jiffies) */
        u32 hash;
};

I had not considered removing last_qtail or its implication of packet
reordering at
this time, so I had both last_qtail and last_active in the structure
(have to understand
this better).

and:
#define RFS_ACCEL_FLOW_TIMEOUT (HZ / 4)
static bool rps_flow_is_active(struct rps_dev_flow *rflow, unsigned int cpu)
{
    /*
     * Check if the flow is for a valid, online CPU and if the current
time is before
       the flow's expiration time.
     */
    return cpu < nr_cpu_ids &&
              time_before(jiffies, rflow->last_use + RFS_ACCEL_FLOW_TIMEOUT);
}

Please let me know if this approach is correct, and whether it should
be a separate patch.

Thanks,
- Krishna