linux-kernel - Re: [PATCH] staging: unisys: use common return path

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]

Date:	Tue, 1 Dec 2015 18:57:24 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Ben Romer <benjamin.romer@...sys.com>
Cc:	Sudip Mukherjee <sudipm.mukherjee@...il.com>,
	devel@...verdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	sparmaintainer@...sys.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: unisys: use common return path

On Tue, Dec 01, 2015 at 09:54:50AM -0500, Ben Romer wrote:
> On 12/01/2015 03:00 AM, Dan Carpenter wrote:
> >Doing One Err style error handling is often a mistake but it's ok here.
> 
> Why is it okay here? I don't understand why this function would be
> any different than the other places where the code used a goto.

What I meant was that I'm generally opposed to "common exit paths".
Mixing all the exit paths together often makes the code more complicated
and leads to errors.  That makes sense from a common sense perspective
that doing many things is more difficult than doing one thing?  Anyway
it's easy enough to verify empirically that this style is bug prone.

On the other hand there are times where all exit paths need to unlock or
to free a variable and in those cases using a common exit path makes
sense.  Just don't standardize on "Every function should only have a
single return".

> 
> If we *have* to change it

I don't think we have to change it at all.  Using direct returns makes
finding locking bugs easier for static checkers.

> I would prefer that we not add a goto and
> instead add an additional boolean local variable to control
> serverdown completion. That's less complex and makes the intent
> clear.
> 

I don't really like this but I also don't care because the function is
short.  The reason I don't like is because it force you to scroll back
up and ideally you could understand a function by reading it from top to
bottom without scrolling backwards.

> like this:
> 
> visornic_serverdown(struct visornic_devdata *devdata,
> 		    visorbus_state_complete_func complete_func)
> {
> 	unsigned long flags;
> 	int retval = 0;
> 	bool complete_serverdown = false;
> 
> 	spin_lock_irqsave(&devdata->priv_lock, flags);
> 	if (!devdata->server_down && !devdata->server_change_state) {
> 		if (devdata->going_away) {
> 			dev_dbg(&devdata->dev->device,
> 				"%s aborting because device removal pending\n",
> 				__func__);
> 			retval = -ENODEV;
> 		} else {
> 			devdata->server_change_state = true;
> 			devdata->server_down_complete_func = complete_func;
> 			complete_serverdown = true;
> 		}
> 	} else if (devdata->server_change_state) {
> 		dev_dbg(&devdata->dev->device, "%s changing state\n",
> 			__func__);
> 		spin_unlock_irqrestore(&devdata->priv_lock, flags);

This is a bug.

> 		retval = -EINVAL;
> 	} 		
> 	spin_unlock_irqrestore(&devdata->priv_lock, flags);
> 	
> 	if (complete_serverdown)
> 		visornic_serverdown_complete(devdata);

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/