Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Thu, Mar 02, 2023 at 11:51:15AM +0100, Florian Westphal wrote:
> > Ivan Delalande <colona@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
> > > Signed-off-by: Ivan Delalande <colona@xxxxxxxxxx>
> > > ---
> > > 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!
