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: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 ...

> 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