Re: [PATCH] netfilter: ctnetlink: revert to dumping mark regardless of event type

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Mar 02, 2023 at 11:51:15AM +0100, Florian Westphal wrote:
> > Ivan Delalande <colona@arista.com> wrote:
> > > I assume that change was unintentional, we have userspace code that
> > > needs the mark while listening for events like REPLY, DESTROY, etc.
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
> > > Signed-off-by: Ivan Delalande <colona@arista.com>
> > > ---
> > >  net/netfilter/nf_conntrack_netlink.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > > index c11dff91d52d..194822f8f1ee 100644
> > > --- a/net/netfilter/nf_conntrack_netlink.c
> > > +++ b/net/netfilter/nf_conntrack_netlink.c
> > > @@ -831,7 +831,7 @@ ctnetlink_conntrack_event(unsigned int events, const struct nf_ct_event *item)
> > >  	}
> > >  
> > >  #ifdef CONFIG_NF_CONNTRACK_MARK
> > > -	if (events & (1 << IPCT_MARK) &&
> > > +	if ((events & (1 << IPCT_MARK) || READ_ONCE(ct->mark)) &&
> > >  	    ctnetlink_dump_mark(skb, ct) < 0)
> > >  		goto nla_put_failure;
> > 
> > Probably better to just drop the event bit test?
> > 
> > if (ctnetlink_dump_mark(skb, ct) < 0)
> > 	goto nla_put_failure;
> 
> Looks good to me, but 1feeae071507 also changes behaviour in another
> way: if ct->mark is reset to zero (being non-zero before), the event
> will not show mark=0 which is probably relevant to userspace. It is a
> strange corner case though.

Oh, right.

> Maybe this?
> 
>         if (ctnetlink_dump_mark(skb, ct, events & (1 << IPCT_MARK)))
>                 goto nla_put_failure;
> 
> then:
> 
> static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct, bool dump)
> {
>         u32 mark = READ_ONCE(ct->mark);
> 
>         if (!mark && !dump)
>                 return 0;

LGTM, Ivan, can you submit a v2 with Pablos suggested change?

Thanks!