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.
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;
then in __ctnetlink_glue_build() and ctnetlink_dump_info():
ctnetlink_dump_mark(..., true)
It seems 1feeae071507 also changed conntrack -L, before my update,
mark=0 was included in the listing.
Probably a good chance to fix these subtle changes.
