linux-kernel - Re: [PATCH] befs: iget_locked() doesn't return an ERR

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140116055357.GW10323@ZenIV.linux.org.uk>
Date:	Thu, 16 Jan 2014 05:53:57 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Rakesh Pandit <rakesh@...era.com>
Cc:	ebiederm@...ssion.com, keescook@...omium.org,
	serge.hallyn@...onical.com, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] befs: iget_locked() doesn't return an ERR_PTR

On Wed, Jan 15, 2014 at 07:58:28PM +0200, Rakesh Pandit wrote:
> Also fix befs_iget return value if iget_locked fails.

Applied, but I really wonder if we'd made a mistake with iget{5}_locked()
API - perhaps the calling conventions returning ERR_PTR(-ENOMEM) would've
been better...  Most of the callers are immediately followed by
	if (!inode)
		return ERR_PTR(-ENOMEM);
and in fact something like
inode_is_new(inode) = !ERR_PTR(inode) && (inode->i_flags & I_NEW)
might've killed even more boilerplate in callers, so it might make
sense to introduce a new variants of iget{,5}_locked that would
return ERR_PTR(-ENOMEM) on out-of-memory and mark the original variants
obsolete after a while...

Anyway, for now your patch is obviously the right fix.

BTW, the callers of iget{,5}_locked() that do not do that return
ERR_PTR(-ENOMEM) on NULL thing are worth looking into:
	* sysfs_get_inode() - callers of _that_ do the same thing; might
as well have done it there.
	* nilfs_iget_locked() - ditto.
	* fuse_iget() - ditto.
	* btrfs_iget_locked() - ditto.
	* udf_iget() - callers galore, with very inconsistent error values
propagated on that failure (EACCES, EIO, etc.); quite a few of those would
be better off with ENOMEM (e.g. ->lookup() failing with EACCES on oom isn't
right).
	* __ecryptfs_get_inode() - EACCES instead of ENOMEM, for no good
reason I can see...
	* gfs2_iget() - ENOBUFS instead of ENOMEM.  ENOBUFS is 
"No buffer space available (POSIX.1 (XSI STREAMS option))" and since
we don't support STREAMS it's probably fair game, but... what the hell?
	* ll_iget() - this one definitely ought to return ERR_PTR(), since
it does that on other errors; callers check for both.  One of them converts
NULL to ERR_PTR(-ENOMEM), another (in mount guts) treats it as EBADF (again,
very odd return value when there's no file descriptors in sight).
	* befs_iget() - the one you'd caught.
	* hfs_iget() - one caller treats it as EACCES, another - as EINVAL.
To make things more complicated, there's another failure exit, which probably
should be EINVAL.  Overall, I'd make it return ERR_PTR()...
	* cifs_iget() - probably would be better off with ERR_PTR(); most
of the callers treat NULL as ENOMEM, but cifs_posix_mkdir() is really
odd in that respect...  No sure what's going on there.
	* hfs_btree_open() - callers end up treating that as EIO (and do that
to kmalloc() failures right in there)
	* hpfs_lookup() - treats all errors as ENOENT, that one included...
	* hpfs_fill_super() - same, but treats everything as EINVAL
	* bdget() and bdget_disk() - a bunch of callers forget to check for
allocation failures at all; some are doing that legitimately, some are
probably oopsable...
--
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/