[bug report] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[bug report] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Dan Carpenter]

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'

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?

    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.

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

return 0.

    580 }

regards,
dan carpenter

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

[Lance Yang]

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

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

[Dan Carpenter]

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
assume that proc_dou8vec_minmax() returns positive values on success
and that's why we are returning ret.  I know that I had to check.

regards,
dan carpenter

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

[Lance Yang]

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

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

[Lance Yang]

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
> 

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

[Dan Carpenter]

On Mon, Aug 04, 2025 at 05:57:09PM +0800, 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 ...
> 
> > 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?

The ret check there should either be the way you wrote it or:

	if (ret || !write)

either one is fine.  Adding a "!= 0" is not really idiomatic because
ret is not a number, it's an error code.

If you have the cross function database, then Smatch knows that ret
can't be positive...  I build with the DB on my desktop but the cross
function DB doesn't scale well enough to be usable by the zero-day bot
so I see this warning on my desktop but the zero-day bot will not
print a warning.

regards,
dan carpenter

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

[Lance Yang]

On 2025/8/4 18:10, Dan Carpenter wrote:
> On Mon, Aug 04, 2025 at 05:57:09PM +0800, 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 ...
>>
>>> 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?
> 
> The ret check there should either be the way you wrote it or:
> 
> 	if (ret || !write)
> 
> either one is fine.  Adding a "!= 0" is not really idiomatic because
> ret is not a number, it's an error code.

Yes. "if (ret || !write)" is much more idiomatic ;)

> If you have the cross function database, then Smatch knows that ret
> can't be positive...  I build with the DB on my desktop but the cross
> function DB doesn't scale well enough to be usable by the zero-day bot
> so I see this warning on my desktop but the zero-day bot will not
> print a warning.

Ah, got it. Would you be interested in sending a patch for it?

Thanks,
Lance