Re: [bug report] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang <lance.yang@linux.dev>]

On 2025/8/4 16:21, Dan Carpenter wrote:
> Hello Lance Yang,
> 
> Commit e89a68046687 ("netfilter: load nf_log_syslog on enabling
> nf_conntrack_log_invalid") from May 26, 2025 (linux-next), leads to
> the following Smatch static checker warning:
> 
> 	net/netfilter/nf_conntrack_standalone.c:575 nf_conntrack_log_invalid_sysctl()
> 	warn: missing error code? 'ret'

Thanks for pointing this out!

> 
> net/netfilter/nf_conntrack_standalone.c
>      559 static int
>      560 nf_conntrack_log_invalid_sysctl(const struct ctl_table *table, int write,
>      561                                 void *buffer, size_t *lenp, loff_t *ppos)
>      562 {
>      563         int ret, i;
>      564
>      565         ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
>      566         if (ret < 0 || !write)
>      567                 return ret;
>      568
>      569         if (*(u8 *)table->data == 0)
>      570                 return ret;
> 
> return 0?

That's a good question. proc_dou8vec_minmax() returns 0 on a successful
write. So when a user writes '0' to disable the feature, ret is already 0.
Returning it is the correct behavior to signal success.

> 
>      571
>      572         /* Load nf_log_syslog only if no logger is currently registered */
>      573         for (i = 0; i < NFPROTO_NUMPROTO; i++) {
>      574                 if (nf_log_is_registered(i))
> --> 575                         return ret;
> 
> This feels like it should be return -EBUSY?  Or potentially return 0.

We simply return ret (which is 0) to signal success, as no further action
(like loading the nf_log_syslog module) is needed.

> 
>      576         }
>      577         request_module("%s", "nf_log_syslog");
>      578
>      579         return ret;
> 
> return 0.

It's 0 as well.

Emm... do you know a way to make the Smatch static checker happy?

Thanks,
Lance

> 
>      580 }
> 
> regards,
> dan carpenter