Re: Wrong __setup() callbacks return values and /init's environment pollution

[Randy Dunlap]

Hi--

I have had this in my todo bucket for too long now...



On 12/25/20 09:20, Igor Zhbanov wrote:
> It seems that nobody knows how to write __setup() callback functions for
> parsing the command line parameters. And there are no documentation or
> comments about best practices.
> 
> Despite being declared obsolete __setup() is used about 435 times in the
> kernel and 51 of them (~11.2%) are erroneous in the sense of returning
> incorrect value (0) resulting in the /init process environment pollution.
> 
> Initially it was mentioned that the callback function should return 1 when
> the parameter value is parsed and consumed successfully, or return 0 to
> keep the unparsed option as init's environment variable.
> 
> But there are no comments or documentation about it, so developers often
> always returning 0 (as it is typically expected in other kernel functions)
> or returning -EINVAL on parse error. All these cases resulting in init's
> environment pollution (which is limited only to 32 variables in the config).
> 
> Even the original usage when 0 is returned on the parse error is questionable
> now. There are no (or not so many) parameter names clashes between different
> kernel subsystems, as well as there not so many parameters to be passed to
> init/systemd that could be interpreted as the kernel parameters. So if a
> kernel module sure that this is its own parameter, may be it would be better
> to always return 1 and consume it, even it is malformed.
> 
> Also there are no recommendations on whether to print a warning when
> incorrect parameter is passed. Some of the functions print a warning on
> incorrect values; some are silently proceeding with the default values.
> Some are even calling panic() on incorrect parameters (e.g.
> setup_time_travel() -> time_travel_connect_external()). So there is no
> consistency on what behavior for handling incorrect parameters values are
> recommended.
> 
> I've tried to categorize all of the __setup() usages. And here is the zoo:
> 
> Functions always returning 1 (right):
>   (About 324)

[cuts]


> 
> Functions always returning 0 (wrong):

> 
> Functions returning 1 on error, 0 on success (wrong):

> 
> Functions returning -errrno (-EINVAL mostly) on error, 0 on success (wrong):

> 
> Functions returning -EINVAL on error, 1 on success (wrong):

> 
> Functions that don't have the return statement at all (wrong):

> 
> So it would be better to document at least how a __setup() callback handler
> should behave regarding the return value, whether to print a warning on parse
> error. And the functions not returning 0 on error and 1 on success must be
> fixed.

Then how about starting with some documentation?
Can you send some?

> Also to avoid init's processes pollution may be it is even better to simply
> always returning 1 even on parse errors, because parameters names collisions
> are very infrequent.


thanks.
-- 
~Randy