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

[netdev-bot+sashiko]

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org