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
