linux-kernel - Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk

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: <20160119104051.21d8e2ac@endymion.delvare>
Date:	Tue, 19 Jan 2016 10:40:51 +0100
From:	Jean Delvare <jdelvare@...e.de>
To:	Pali Rohár <pali.rohar@...il.com>
Cc:	Andy Lutomirski <luto@...nel.org>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return
 real error codes

On Tue, 19 Jan 2016 10:07:36 +0100, Pali Rohár wrote:
> On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:
> > Hi Pali,
> > 
> > On Tue, 19 Jan 2016 09:36:33 +0100, Pali Rohár wrote:
> > > On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
> > > > > @@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
> > > > >  	u8 *buf;
> > > > >  
> > > > >  	if (!dmi_available)
> > > > > -		return -1;
> > > > > +		return -ENOENT;
> > > > 
> > > > -ENOSYS would seem more appropriate?
> > > 
> > > IIRC -ENOSYS is for non implemented syscalls.
> > 
> > I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when
> > a specific subsystem is not included. Not related to syscalls at all.
> > This is what lead to my suggestion.
> 
> https://lkml.org/lkml/2014/8/22/492

Thanks for the pointer, I wasn't aware of that.

It really should be documented. No, checkpatch.pl isn't documentation.

Also the commit sadly doesn't say why using ENOSYS in other contexts is
considered a bad thing. What actual trouble did it cause?

Are the current presumably incorrect uses of ENOSYS ultimately going to
be fixed? If not, I see no point in preventing other use cases.

Also what about errno(3)? It says ENOSYS is "Function not implemented"
- no mention to syscalls. And glibc agrees (set errno to ENOSYS and
call perror, it says "Function not implemented.") In contradiction with
the proposed rule.

Back to the patch under review, I would argue that ENOENT is solely for
file-related operations (after all the man page says "No such file or
directory"), which is why I did not like it. Then what is left to mean
"function not available"? ENOTSUP?

Whatever the answer is, it should also be documented and added to the
checkpatch warning message (if we stick to that plan...) If you tell
people "don't use this" without telling them what to use instead, each
of us will come up with something different and this will lead to
inconsistency. Which is worse than everybody using the same (maybe
suboptimal) error code.

-- 
Jean Delvare
SUSE L3 Support