netdev - Re: [PATCH net-next v2 4/9] hsr: Implement more robust duplicate discard for PRP

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: <aYHtSP-Pl1mTpkJC@thinkpad>
Date: Tue, 3 Feb 2026 13:42:48 +0100
From: Felix Maurer <fmaurer@...hat.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, jkarrenpalo@...il.com, tglx@...utronix.de,
	mingo@...nel.org, allison.henderson@...cle.com, petrm@...dia.com,
	antonio@...nvpn.net, Steffen Lindner <steffen.lindner@...abb.com>
Subject: Re: [PATCH net-next v2 4/9] hsr: Implement more robust duplicate
 discard for PRP

On Tue, Feb 03, 2026 at 12:57:19PM +0100, Sebastian Andrzej Siewior wrote:
> On 2026-02-03 11:23:57 [+0100], Felix Maurer wrote:
> > On Mon, Feb 02, 2026 at 05:57:02PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2026-01-28 19:37:23 [+0100], Felix Maurer wrote:
> > > > > > +	if (!block) {
> > > > > > +		block = &node->block_buf[node->next_block];
> > > > > > +		hsr_forget_seq_block(node, block);
> > > > > > +
> > > > > > +		memset(block, 0, sizeof(*block));
> > > > > > +		block->time = jiffies;
> > > > > > +		block->block_idx = block_idx;
> > > > > > +
> > > > > > +		res = xa_store(&node->seq_blocks, block_idx, block, GFP_ATOMIC);
> > > > > > +		if (xa_is_err(res))
> > > > >
> > > > > Hi Felix,
> > > > >
> > > > > I ran Claude Code over this with review-prompts [1] and it flags
> > > > > that in the error path above, the following is needed so that the
> > > > > block can be re-used.
> > > > >
> > > > > 			block->time = 0;
> > > >
> > > > I agree. It would nonetheless be reused at some point, but not setting
> > > > time = 0 may lead to an unexpected block getting removed when it is
> > > > reused (or at least an attempt to do so). I'll fix this in v3.
> > >
> > > Not sure I follow. If xa_store() fails then the block is not added to
> > > the "sequence-blocks" and it will be attempted once the next packet is
> > > received, right?. Otherwise node->next_block is not updated at which
> > > point this block will be added twice which sounds worse.
> >
> > Yes, it will be attempted again on the next frame that needs a new
> > block. But every attempt to recycle the next_block starts with calling
> > hsr_forget_seq_block(), which tries to xa_erase() the block if time!=0.
> > Generally, time==0 is the marker that a block is not stored in the
> > xarray. So for consistency it's correct to set time=0 if the block can't
> > be added to the xarray, even though I don't think it would cause any
> > trouble at the moment if the time is not reset.
>
> My point is that the block will not be recycled if it failed to be added
> here. It remains the "next" block to be used and added to the list if it
> fails that xa_store().

Hm, I'm not sure I follow. Do you argue that the code already in the
patch is incorrect or that it would be incorrect with Simon's
suggestion?

Yes, in case the xa_store() fails, the current "next" block remains the
"next" block. But we also definitely erased it from the xarray already.
With the next frame, we attempt to recycle/insert the block again. I
don't think that can lead to the same block being used twice?

I understood Simon's suggestion this way: in the error path, *also*
reset block->time to 0. We'd still return NULL in that case and,
crucially, don't advance the next_block. IMHO, that only makes the data
more consistent (blocks in the buffer that are not in the xarray always
have time=0) and prevents that we try to remove a non-existing entry
from the xarray on the next frame when we try again to recycle the
current "next" block.

Thanks,
   Felix