Re: [PATCH net 2/6] net: openvswitch: conntrack: remove 'add_helper' dead code

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

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Concern: after this patch the comment above the helper call reads
>   '- nf_conntrack_in() was executed above ("!cached"), or - When…
> 
> --
> 
> net: openvswitch: conntrack: remove 'add_helper' dead code
> 
> This drops the add_helper variable in __ovs_ct_lookup(), since it is only
> ever set for unconfirmed connections while it is only read on the
> confirmed branch of the helper-call condition.  The helper-call condition
> is simplified to nf_ct_is_confirmed(ct) ? !cached : info->commit.
> 
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index a733029c28dd0..c20f096eef40e 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
> 
> [ ... ]
> 
>> @@ -821,13 +818,10 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>>  		}
>>  
>>  		/* Call the helper only if:
>> -		 * - nf_conntrack_in() was executed above ("!cached") or a
>> -		 *   helper was just attached ("add_helper") for a confirmed
>> -		 *   connection, or
>> +		 * - nf_conntrack_in() was executed above ("!cached"), or
>>  		 * - When committing an unconfirmed connection.
>>  		 */
>> -		if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
>> -					      info->commit)) {
>> +		if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
> 
> [Severity: Low]
> This isn't a bug, but does the reworded comment still match the condition?
> 
> The first bullet now reads as an unqualified statement:
> 
> 	/* Call the helper only if:
> 	 * - nf_conntrack_in() was executed above ("!cached"), or
> 	 * - When committing an unconfirmed connection.
> 	 */
> 
> while the code only honours "!cached" on the confirmed side of the
> ternary:
> 
> 	if ((nf_ct_is_confirmed(ct) ? !cached : info->commit)) {
> 
> For a non-committing ct() action on a new packet in __ovs_ct_lookup(),
> cached is false, the entry returned by nf_ct_get() is not yet confirmed
> and info->commit is 0, so nf_ct_helper() is not called even though the
> first bullet says it should be.
> 
> Worth noting that the following patch in the series, "net: openvswitch:
> conntrack: fix helper UAF due to extensions realloc", rewrites both the
> comment and the condition to:
> 
> 	/* Call the helper only if nf_conntrack_in() was executed
> 	 * above ("!cached").
> 	 *
> 	 * For unconfirmed connections it will be called later during
> 	 * commit as we need to have all the other extensions allocated
> 	 * before the call.
> 	 */
> 	if (nf_ct_is_confirmed(ct) && !cached) {
> 
> so the mismatch only exists in this intermediate commit.  Could the
> deferral note be pulled into this patch to keep each commit
> self-consistent, given this one is tagged for stable?
Comments do not need to spell out verbatim what the code is doing,
it is pointless for the most part.  The current comment, IMO, is
conveying the intention clear enough.  It is also re-written in the
very next patch.

Best regards, Ilya Maximets.