Re: [PATCH net 06/12] netfilter: nf_conntrack_expect: honor expectation helper field

[Ilya Maximets <i.maximets@ovn.org>]

On 5/5/26 1:16 AM, Pablo Neira Ayuso wrote:
> Hi Ilya,
> 
> On Mon, May 04, 2026 at 02:19:20PM +0200, Ilya Maximets wrote:
>> On 5/1/26 12:37 PM, Pablo Neira Ayuso wrote:
>>> Hi Ilya,
>>>
>>> On Thu, Apr 30, 2026 at 10:58:38PM +0200, Ilya Maximets wrote:
>>>> On 3/26/26 1:51 PM, Pablo Neira Ayuso wrote:
>>>>> The expectation helper field is mostly unused. As a result, the
>>>>> netfilter codebase relies on accessing the helper through exp->master.
>>>>>
>>>>> Always set on the expectation helper field so it can be used to reach
>>>>> the helper.
>>>>>
>>>>> nf_ct_expect_init() is called from packet path where the skb owns
>>>>> the ct object, therefore accessing exp->master for the newly created
>>>>> expectation is safe. This saves a lot of updates in all callsites
>>>>> to pass the ct object as parameter to nf_ct_expect_init().
>>>>>
>>>>> This is a preparation patches for follow up fixes.
>>>>>
>>>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>>>> ---
>>>>
>>>> Hi, Pablo and Florian.
>>>>
>>>> I was investigating FTP test failures in OVS with 7.0 kernel and bisected
>>>> the issue down to this commit.  AFAIU, with this change all the related
>>>> connections over time gain their parents' helpers,.  This is causing a change
>>>> visible to the userspace, because FTP data connections are now reported to
>>>> have helpers in the conntrack dump:
>>>>
>>>> # conntrack -L
>>>> tcp      6 119 TIME_WAIT src=10.1.1.1 dst=10.1.1.2 sport=59534 dport=21 \
>>>>                          src=10.1.1.2 dst=10.1.1.1 sport=21    dport=59534 \
>>>>            [ASSURED] mark=0 helper=ftp use=2
>>>> tcp      6 119 TIME_WAIT src=10.1.1.2 dst=10.1.1.1 sport=52709 dport=52381 \
>>>>                          src=10.1.1.1 dst=10.1.1.2 sport=52381 dport=52709 \
>>>>            [ASSURED] mark=0 helper=ftp use=1
>>>>
>>>> Before this commit only the control connection had helper=ftp reported in
>>>> the dump.  The traffic seems to work fine, but our tests fail because we
>>>> do not expect the helper attached.
>>>>
>>>> AFAIU, it's generally not something that should be happening, as helpers
>>>> on data connections do not really make much sense.  But I'm just trying to
>>>> figure out if you would consider this as a regression and fix in the kernel
>>>> or if we should adjust our userspace components for this new dump content,
>>>> which would not be very straightforward to do if we want to be able to run
>>>> tests on both old and the new versions.
>>>>
>>>> What do you think?
>>>
>>> It seems previous behaviour to 9c42bc9db90a was inconsistent, ie. only
>>> the h323 helper sets on exp->helper, then it shows helper= in expected
>>> connections via ctnetlink. I guess this is for debugging given that
>>> h323 is actually a family of helpers.
>>>
>>> To consistently skip dumping this for expected connections, probably
>>> this is the way to do:
>>>
>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conn
>>> index eda5fe4a75c8..9491ae9e080e 100644
>>> --- a/net/netfilter/nf_conntrack_netlink.c
>>> +++ b/net/netfilter/nf_conntrack_netlink.c
>>> @@ -226,7 +226,7 @@ static int ctnetlink_dump_helpinfo(struct sk_buff *sk
>>>         const struct nf_conn_help *help = nfct_help(ct);
>>>         struct nf_conntrack_helper *helper;
>>>  
>>> -       if (!help)
>>> +       if (!help || ct->status & IPS_EXPECTED)
>>>                 return 0;
>>>  
>>>         rcu_read_lock();
>>
>> I'm not sure.  I tried this change and it fixed one case but broke another.
>> Looking at what we're testing, the old behavior (at least for FTP) was:
>> "if helper was committed - report it, if not - don't".  i.e. it's not really
>> about the connection being expected it's about if the user committed the
>> helper for the connection or not.
>>
>> Let me explain a few scenarios that we have in the OVS system tests and what
>> I see with the old kernel (6.19), the new (7.0) and the patch above.
>>
>> A) The first scenario has the following OpenFlow rules (simplified):
>>
>>   table=0,in_port=1,tcp,action=ct(alg=ftp,commit),2
>>   table=0,in_port=2,tcp,action=ct(table=1)
>>   table=1,in_port=2,tcp,ct_state=+trk+est,action=1
>>   table=1,in_port=2,tcp,ct_state=+trk+rel,action=1
>>
>> This set of rule blindly commits every packet coming from port 1 with the
>> helper and sends to port 2.  Packets from port 2 are passed through ct and
>> only related or established traffic is passed to port 1.  This is a very
>> rudimentary setup that users can make to allow ftp from port 1 towards port 2,
>> but not in the opposite direction.
>>
>> For this scenario regardless of the kernel version or the patch above I see
>> that both the data and the control connections have a helper reported in the
>> ctnetlink dump.
> 
> This ruleset then is attached the conntrack helper to data connection,
> that is, ALG is inspecting the FTP data connection but it will just
> find no patterns because it is only the FTP control connection that
> creates expectations?

Yes.  It's just a "lazy" way to make the traffic work, we do not expect
the helper on the data connection to do anything useful in this scenario.

Best regards, Ilya Maximets.