netdev - Re: [PATCH net 2/5] netfilter: nf_tables: add position handle in event notification

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: <YVVnJ+Rv36/aF3+u@salvia>
Date:   Thu, 30 Sep 2021 09:28:39 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH net 2/5] netfilter: nf_tables: add position handle in
 event notification

Hi Jakub,

On Wed, Sep 29, 2021 at 07:19:53PM -0700, Jakub Kicinski wrote:
> On Thu, 30 Sep 2021 01:04:57 +0200 Pablo Neira Ayuso wrote:
> > Add position handle to allow to identify the rule location from netlink
> > events. Otherwise, userspace cannot incrementally update a userspace
> > cache through monitoring events.
> > 
> > Skip handle dump if the rule has been either inserted (at the beginning
> > of the ruleset) or appended (at the end of the ruleset), the
> > NLM_F_APPEND netlink flag is sufficient in these two cases.
> > 
> > Handle NLM_F_REPLACE as NLM_F_APPEND since the rule replacement
> > expansion appends it after the specified rule handle.
> > 
> > Fixes: 96518518cc41 ("netfilter: add nftables")
> > Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
> 
> Let me defer to Dave on this one. Krzysztof K recently provided us with
> this quote:
> 
> "One thing that does bother [Linus] is developers who send him fixes in the
> -rc2 or -rc3 time frame for things that never worked in the first place.
> If something never worked, then the fact that it doesn't work now is not
> a regression, so the fixes should just wait for the next merge window.
> Those fixes are, after all, essentially development work."
> 
> 	https://lwn.net/Articles/705245/
> 
> Maybe the thinking has evolved since, but this patch strikes me as odd.
> We forgot to put an attribute in netlink 8 years ago, and suddenly it's
> urgent to fill it in?  Something does not connect for me, certainly the
> commit message should have explained things better...

Reasonable, but in this particular case I cannot fix userspace monitor
mode without this patch.

A user reported that 'nft insert rule...' shows as 'nft add rule...'
in 'nft monitor'.

Then if 'nft add rule x y position 10...' is used to add a rule at a
given position, then it does not show the 'position 10' so the user
is just getting a 'add rule x y' which means append it at the end.

Same thing happens with 'create table x', it shows as 'add table x'.

Noone noticed the missing flags in the event notification path so far.

I can place this into net-next, yes, but this is only going to delay
things before I can ask for including this in -stable, meanwhile users
will keep getting misleading event notification for these cases.

Thanks.