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

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

On 2025/8/4 17:57, Lance Yang wrote:
> 
> 
> On 2025/8/4 17:24, Dan Carpenter wrote:
>> On Mon, Aug 04, 2025 at 05:05:32PM +0800, Lance Yang wrote:
>>>
>>>
>>> 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?
>>>
>>
>> Returning 0 would make the code so much more clear.  Readers probably
> 
> Yep, I see your point ;)
> 
>> assume that proc_dou8vec_minmax() returns positive values on success
> 
> IIUC, proc_dou8vec_minmax only returns 0 for success or a negative
> error code, so there's no positive value case here ...

Correction:

IIUC, proc_dou8vec_minmax only returns 0 for success or a negative
error code on a write, so there's no positive value case here ...

> 
>> and that's why we are returning ret.  I know that I had to check.
>>
> 
> It should make the code clearer and also make the Smatch checker happy:
> 
>      ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
>      - if (ret < 0 || !write)
>      + if (ret != 0 || !write)
>          return ret;
> 
> By checking for "ret != 0", it becomes explicit that ret can only be
> zero after that. wdyt?
> 
> Also, if you'd like, please feel free to send a patch for it ;p
> 
> Thanks,
> Lance
>