linux-kernel - Re: [patch 04/14] gfs2: dont call permission()

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:	Fri, 23 May 2008 11:18:50 +0100
From:	Steven Whitehouse <swhiteho@...hat.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	hch@...radead.org, linux-fsdevel@...r.kernel.org,
	viro@...IV.linux.org.uk, linux-kernel@...r.kernel.org
Subject: Re: [patch 04/14] gfs2: dont call permission()

Hi,

On Fri, 2008-05-23 at 11:48 +0200, Miklos Szeredi wrote:
> > Bad idea.  You're duplicating bits out of permission for no good
> > reason.  I spent quite a bit of effort to make sure we don't have
> > this duplicated logic around.
> 
> In this case you are wrong.  Look at the ugly conditional locking
> gfs2_permission() does, which is probably due to the fact that it's
> doing a recursion via calling permission() from inside already locked
> parts in the filesystem.
> 
> That's _much_ worse than a duplicated IS_IMMUTABLE() call.  Which btw,
> is a filesystem implementation detail: it needs to re-check the
> immutability of the file after it has been locked.  I'm not even sure
> it's strictly needed.  Steven?
> 
Given the fact that (a) its only a very minor change and (b) as soon as
we have a solution to what we really want to do:

 - inode/file operation:
   - Do lookup via VFS
   - Get GFS2 glock
     - Do perm check via VFS
     - Do actual operation
   - Drop GFS2 glock

as opposed to the current situation of:

 - Do lookup via VFS:
   - Get GFS2 glock
   - Do perm check
   - Drop GFS2 glock
 - inode/file operation:
   - Get GFS2 glock
   - Recheck perms
   - Do the actual operation
   - Drop GFS2 glock

then the rechecking of perms will no longer be required anyway. I'm
inclined to agree with Miklos and say that its ok, but strictly on the
basis that its days are already numbered.

> Generally this sort of recursion through the VFS is ugly and
> unnecessary, it's much better to provide helper for what the VFS is
> doing if there's a lot of duplication.  But in this case there's
> really no point in that at all.
> 
> Miklos

Indeed I've spent a lot of time tracking down these cases and at least
now its possible to see them all by greping for
gfs2_glock_is_locked_by_me() in gfs2, whereas previously these cases
were hidden and less obvious,

Steve.


--
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/