linux-kernel - Re: scsi: ses: Move a label in ses_enclosure_data

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: Sat, 30 Dec 2023 08:45:59 -0500
From: James Bottomley <jejb@...ux.ibm.com>
To: Markus Elfring <Markus.Elfring@....de>, linux-scsi@...r.kernel.org,
        kernel-janitors@...r.kernel.org,
        "Martin K. Petersen"
	 <martin.petersen@...cle.com>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: scsi: ses: Move a label in ses_enclosure_data_process()

On Sat, 2023-12-30 at 14:36 +0100, Markus Elfring wrote:
> > > You probably know some advices from another information source.
> > > 
> > > https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources
> > > 
> > 
> > Yes, but it's about using staged deallocation at the end of the
> > function instead of in the if loops.  That's to *simplify* the exit
> > chain and make the error legs less error prone because the teardown
> > isn't repeated in if bodies.  It has no bearing on what you just
> > tried to do.
> 
> I got the impression that there is a general conflict involved
> according to different programming styles.

There isn't; you simply seem to have misunderstood what the above 
resource is actually saying.

> > > >                                               If coccinelle
> > > > suddenly thinks this is a problem, it's coccinelle that needs
> > > > fixing.
> > > 
> > > This software tool can help to point source code places out for
> > > further considerations. The search patterns are evolving
> > > accordingly.
> > 
> > The pattern is wrong because kfree(NULL) exists as a teardown
> > simplification.
> 
> It might be convenient to view in this way.
> 
> If you would dare to follow advice from goto chains in a strict way,
> I imagine that you can tend to stress the attention for more useful
> data processing a bit more than such a redundant function call.

It's about maintainability and simplicity.  Eliminating kfree(NULL)
doesn't simplify most code, it just makes the exit paths more complex
as I've said for the third time now.  It could possibly be done in
extremely performance critical situations for good and well documented
reason, but that's only a tiny fraction of the kernel and it certainly
doesn't apply here.

James