netdev - Re: [PATCH] net: sxgbe: fix return value of _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d397d64f-8ae6-2713-d71c-465ae71baebe@infradead.org>
Date:   Thu, 24 Feb 2022 23:30:42 -0800
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, patches@...ts.linux.dev,
        Igor Zhbanov <i.zhbanov@...russia.ru>,
        Siva Reddy <siva.kallam@...sung.com>,
        Girish K S <ks.giri@...sung.com>,
        Byungho An <bh74.an@...sung.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] net: sxgbe: fix return value of __setup handler



On 2/24/22 21:43, Jakub Kicinski wrote:
> On Wed, 23 Feb 2022 19:35:28 -0800 Randy Dunlap wrote:
>> __setup() handlers should return 1 on success, i.e., the parameter
>> has been handled. A return of 0 causes the "option=value" string to be
>> added to init's environment strings, polluting it.
> 
> Meaning early_param_on_off() also returns the wrong thing?
> Or that's different?

early_param() and its varieties are different -- 0 for success, else error.

>> Fixes: acc18c147b22 ("net: sxgbe: add EEE(Energy Efficient Ethernet) for Samsung sxgbe")
>> Fixes: 1edb9ca69e8a ("net: sxgbe: add basic framework for Samsung 10Gb ethernet driver")
>> Signed-off-by: Randy Dunlap <rdunlap@...radead.org>
>> Reported-by: Igor Zhbanov <i.zhbanov@...russia.ru>
>> Link: lore.kernel.org/r/64644a2f-4a20-bab3-1e15-3b2cdd0defe3@...russia.ru
>>
>> --- linux-next-20220223.orig/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
>> +++ linux-next-20220223/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
>> @@ -2285,18 +2285,18 @@ static int __init sxgbe_cmdline_opt(char
>>  	char *opt;
>>  
>>  	if (!str || !*str)
>> -		return -EINVAL;
>> +		return 1;
>>  	while ((opt = strsep(&str, ",")) != NULL) {
>>  		if (!strncmp(opt, "eee_timer:", 10)) {
>>  			if (kstrtoint(opt + 10, 0, &eee_timer))
>>  				goto err;
>>  		}
>>  	}
>> -	return 0;
>> +	return 1;
>>  
>>  err:
>>  	pr_err("%s: ERROR broken module parameter conversion\n", __func__);
>> -	return -EINVAL;
>> +	return 1;
>>  }
>>  
>>  __setup("sxgbeeth=", sxgbe_cmdline_opt);
> 
> Was the option of making __setup() return void considered?
> Sounds like we always want to return 1 so what's the point?

Well, AFAIK __setup() has been around forever (at least 22 years), so No,
I don't think anyone has considered making it void.

Returning 1 or 0 gives kernel parameter writers the option of how error
input is handled, although 0 is usually wrong. :)

-- 
~Randy