[PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Ian Cowan]

Refactor the cedrus_open() function so that there is only one exit to
the function instead of 2. This prevents a future change from preventing
the mutex from being unlocked after a successful exit.

Signed-off-by: Ian Cowan <ian@linux.cowan.aero>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 68b3dcdb5df3..5236d9e4f4e8 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -348,14 +348,14 @@ static int cedrus_open(struct file *file)
 
 	v4l2_fh_add(&ctx->fh);
 
-	mutex_unlock(&dev->dev_mutex);
-
-	return 0;
+	ret = 0;
+	goto succ_unlock;
 
 err_ctrls:
 	v4l2_ctrl_handler_free(&ctx->hdl);
 err_free:
 	kfree(ctx);
+succ_unlock:
 	mutex_unlock(&dev->dev_mutex);
 
 	return ret;
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Dan Carpenter]

On Sat, Apr 23, 2022 at 02:01:11PM -0400, Ian Cowan wrote:
> Refactor the cedrus_open() function so that there is only one exit to
> the function instead of 2. This prevents a future change from preventing
> the mutex from being unlocked after a successful exit.
> 
> Signed-off-by: Ian Cowan <ian@linux.cowan.aero>

No.  You are just making the code ugly and complicated for no reason.

I work in static analysis so I have focussed a lot of attention on
locking bugs.  In real life this theory is totally bogus.  Single exit
paths only cause bugs, they don't prevent bugs.

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 1176 bytes --]

Hi Dan,

On Mon 25 Apr 22, 12:20, Dan Carpenter wrote:
> On Sat, Apr 23, 2022 at 02:01:11PM -0400, Ian Cowan wrote:
> > Refactor the cedrus_open() function so that there is only one exit to
> > the function instead of 2. This prevents a future change from preventing
> > the mutex from being unlocked after a successful exit.
> > 
> > Signed-off-by: Ian Cowan <ian@linux.cowan.aero>
> 
> No.  You are just making the code ugly and complicated for no reason.
> 
> I work in static analysis so I have focussed a lot of attention on
> locking bugs.  In real life this theory is totally bogus.  Single exit
> paths only cause bugs, they don't prevent bugs.

I'm really surprised by this and honestly it feels a bit dogmatic.

It reminds me of CS teachers telling me "gotos are evil and you must
never use them". In practice there are many situations where they make
the code more readable and don't introduce any significant incertainty.

In this instance I don't see what could possible go wrong and I agree it
makes the code more readable.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Dan Carpenter]

On Mon, Apr 25, 2022 at 11:29:30AM +0200, Paul Kocialkowski wrote:
> > 
> > No.  You are just making the code ugly and complicated for no reason.
> > 
> > I work in static analysis so I have focussed a lot of attention on
> > locking bugs.  In real life this theory is totally bogus.  Single exit
> > paths only cause bugs, they don't prevent bugs.
> 
> I'm really surprised by this and honestly it feels a bit dogmatic.
> 
> It reminds me of CS teachers telling me "gotos are evil and you must
> never use them". In practice there are many situations where they make
> the code more readable and don't introduce any significant incertainty.

Gotos are fine.  Backwards gotos are horrible, but sometimes necessary.
But pointless gotos are bad.  And "out" is a bad label name.

	return -ENOMEM;

That's perfectly readable.

	ret = -ENOMEM;
	goto out;

That's vague.  The out label is likely to do nothing or everything.  The
problem with do-nothing gotos are that people forget to set the error
code.  Also it interrupts the reader, now you have to scroll to the
bottom of the function, you have lost your train of thought, and then
you have scroll back and find your place again.

Do-everything gotos are the most bug prone style of error handling.
Imagine the function is trying to do three things.  It fails part way
through.  Now you're trying to undo the second thing which was never
done.  Just moments ago I was just looking at one of these do-everything
bugs where it was using uninitialized memory.

Another problem with do-everything error handling is that eventually it
gets too complicated so people just leave the error handling out.  It's
hard to audit to see if everything is freed.

With static analysis, I'm mostly looking at error handling instead of
success paths paths.  The one error label style is the by far the
worst.

People think single labels will prevent locking bugs.  It doesn't work.
There is two people who use indenting to remind them about locks:

	lock(); {
		frob();
		frob();
		frob();
	} unlock();

And occasionally they still forget to drop the lock before returning on
error paths.  Nothing works for forgot to drop the lock bugs except
static analysis.

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 2976 bytes --]

Hi Dan,

On Mon 25 Apr 22, 13:00, Dan Carpenter wrote:
> On Mon, Apr 25, 2022 at 11:29:30AM +0200, Paul Kocialkowski wrote:
> > > 
> > > No.  You are just making the code ugly and complicated for no reason.
> > > 
> > > I work in static analysis so I have focussed a lot of attention on
> > > locking bugs.  In real life this theory is totally bogus.  Single exit
> > > paths only cause bugs, they don't prevent bugs.
> > 
> > I'm really surprised by this and honestly it feels a bit dogmatic.
> > 
> > It reminds me of CS teachers telling me "gotos are evil and you must
> > never use them". In practice there are many situations where they make
> > the code more readable and don't introduce any significant incertainty.
> 
> Gotos are fine.  Backwards gotos are horrible, but sometimes necessary.
> But pointless gotos are bad.  And "out" is a bad label name.
> 
> 	return -ENOMEM;
> 
> That's perfectly readable.
> 
> 	ret = -ENOMEM;
> 	goto out;
> 
> That's vague.  The out label is likely to do nothing or everything.  The
> problem with do-nothing gotos are that people forget to set the error
> code.  Also it interrupts the reader, now you have to scroll to the
> bottom of the function, you have lost your train of thought, and then
> you have scroll back and find your place again.

Okay I think these are good points, fair enough.

> Do-everything gotos are the most bug prone style of error handling.
> Imagine the function is trying to do three things.  It fails part way
> through.  Now you're trying to undo the second thing which was never
> done.  Just moments ago I was just looking at one of these do-everything
> bugs where it was using uninitialized memory.

So by that you mean having just one label for all error handling instead
of labels for each undo step?

I've also seen conditionals used in error labels to undo stuff.

Would you recommend duplicating error cleanup in each error condition?
It feels like another set of issue on its own, besides the obvious downside
of duplication.

Thanks for the explanation!

Paul

> Another problem with do-everything error handling is that eventually it
> gets too complicated so people just leave the error handling out.  It's
> hard to audit to see if everything is freed.
> 
> With static analysis, I'm mostly looking at error handling instead of
> success paths paths.  The one error label style is the by far the
> worst.
> 
> People think single labels will prevent locking bugs.  It doesn't work.
> There is two people who use indenting to remind them about locks:
> 
> 	lock(); {
> 		frob();
> 		frob();
> 		frob();
> 	} unlock();
> 
> And occasionally they still forget to drop the lock before returning on
> error paths.  Nothing works for forgot to drop the lock bugs except
> static analysis.
> 
> regards,
> dan carpenter

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Dan Carpenter]

On Tue, Apr 26, 2022 at 09:39:03AM +0200, Paul Kocialkowski wrote:
> 
> > Do-everything gotos are the most bug prone style of error handling.
> > Imagine the function is trying to do three things.  It fails part way
> > through.  Now you're trying to undo the second thing which was never
> > done.  Just moments ago I was just looking at one of these do-everything
> > bugs where it was using uninitialized memory.
> 
> So by that you mean having just one label for all error handling instead
> of labels for each undo step?
> 

Yes.  Don't do that.  If you try to free everything, half the stuff is
not allocated so you will undo things which have not been done and it
leads to a bug.

> I've also seen conditionals used in error labels to undo stuff.
> 

I don't understand what you're describing?

> Would you recommend duplicating error cleanup in each error condition?
> It feels like another set of issue on its own, besides the obvious downside
> of duplication.

Let me write a blog about it:

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 1715 bytes --]

Hi Dan,

On Thu 28 Apr 22, 13:26, Dan Carpenter wrote:
> On Tue, Apr 26, 2022 at 09:39:03AM +0200, Paul Kocialkowski wrote:
> > 
> > > Do-everything gotos are the most bug prone style of error handling.
> > > Imagine the function is trying to do three things.  It fails part way
> > > through.  Now you're trying to undo the second thing which was never
> > > done.  Just moments ago I was just looking at one of these do-everything
> > > bugs where it was using uninitialized memory.
> > 
> > So by that you mean having just one label for all error handling instead
> > of labels for each undo step?
> > 
> 
> Yes.  Don't do that.  If you try to free everything, half the stuff is
> not allocated so you will undo things which have not been done and it
> leads to a bug.
>
> > I've also seen conditionals used in error labels to undo stuff.
> > 
> 
> I don't understand what you're describing?

Typically that would look like:

void *foo = NULL;
void *bar = NULL;

foo = alloc(...);
if (!foo)
	goto single_error;

bar = alloc(...);
if (!bar)
	goto single_error;

...

single_error:
	if (bar)
		free(bar);

	if (foo)
		free(foo);

> > Would you recommend duplicating error cleanup in each error condition?
> > It feels like another set of issue on its own, besides the obvious downside
> > of duplication.
> 
> Let me write a blog about it:
> 
> https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

Good writeup, thanks! The part about unwinding loops especially, I've always
wondered about the right way to go about it.

Cheers,

Paul 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Dan Carpenter]

On Sat, Apr 23, 2022 at 02:01:11PM -0400, Ian Cowan wrote:
> Refactor the cedrus_open() function so that there is only one exit to
> the function instead of 2. This prevents a future change from preventing
> the mutex from being unlocked after a successful exit.

Future proofing code is a complicated thing because the future is hard
to predict.

But one rule is that normally the success path gets tested.  No one is
going to forget to drop the lock on the success path.  Or if we do forget,
it likely will get caught quickly and it will be easy to bisect.

Generally it's best to keep the success path and the failure path as
separate as possible.  Avoid interleaving.  It's more readable.  If
there is no cleanup on failure then it's okay to have:

unlock:
	unlock(foo);
	return ret;

But once you have cleanup then put that in a separate cleanup block.

As well Smatch can recognize a typical kernel cleanup block so if you
write it in a weird way then you're disabling the static checker warning
for missing error codes.  I probably am going to write more checks which
are specific to cleanup blocks in the future.

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit

[Jernej Škrabec]

Hi Ian,

Dne sobota, 23. april 2022 ob 20:01:11 CEST je Ian Cowan napisal(a):
> Refactor the cedrus_open() function so that there is only one exit to
> the function instead of 2. This prevents a future change from preventing
> the mutex from being unlocked after a successful exit.
> 
> Signed-off-by: Ian Cowan <ian@linux.cowan.aero>

If this patch would be part of series and "future" would mean next patch, I 
would be ok with that. However, in current form I don't see any benefit in 
changing it. Doing another hop lessens readability IMO. Let us worry about the 
future when/if it comes.

Best regards,
Jernej

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/
media/sunxi/cedrus/cedrus.c
> index 68b3dcdb5df3..5236d9e4f4e8 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -348,14 +348,14 @@ static int cedrus_open(struct file *file)
>  
>  	v4l2_fh_add(&ctx->fh);
>  
> -	mutex_unlock(&dev->dev_mutex);
> -
> -	return 0;
> +	ret = 0;
> +	goto succ_unlock;
>  
>  err_ctrls:
>  	v4l2_ctrl_handler_free(&ctx->hdl);
>  err_free:
>  	kfree(ctx);
> +succ_unlock:
>  	mutex_unlock(&dev->dev_mutex);
>  
>  	return ret;
> -- 
> 2.35.1
> 
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel