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: <20220225075950.7ba5cc62@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Fri, 25 Feb 2022 07:59:50 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Randy Dunlap <rdunlap@...radead.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 Thu, 24 Feb 2022 23:30:42 -0800 Randy Dunlap wrote:
> 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.

Making it an even stronger contender for the worst API ever award ;)

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

Yeah, well, we've been making bus ->remove() functions void IIUC
so why not this. That's separate from fixing the current incorrect
uses tho, so let me apply both of the netdev fixes, thank you!