linux-kernel - Re: [PATCH mmotm] fsnotify: don't slow everything down

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: <20090521094613.GT8633@ZenIV.linux.org.uk>
Date:	Thu, 21 May 2009 10:46:13 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Eric Paris <eparis@...hat.com>
Cc:	Hugh Dickins <hugh@...itas.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Christoph Hellwig <hch@....de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH mmotm] fsnotify: don't slow everything down

On Sun, May 17, 2009 at 11:38:17AM -0400, Eric Paris wrote:

> If people feel strongly I can come up with a system to reuse the inotify
> flag now....  I planned on dropping the old inotify flag in a couple
> releases when I finally evict inotify entirely, it would be a
> performance hit, but I have a feeling a minimal one.  In any case, when
> I push these patches along I'll probably move the new flag rather than
> _COOKIE since long term they won't be 'together'
> 
> Acked-by: Eric Paris <eparis@...hat.com>

OK...  After the last round of review (going by what's in mmotm):
	* it got much better; at least the lifetime rules for generic
structures are sane now and locking seems to be OK.
	* fsnotify() has too many arguments ;-/  It might actually be
worth splitting into for-inode/for-file/etc. cases, and to hell with
code duplication.  Note that adding for-dentry variant would take care
of the file_name argument, so in any case it might be a good idea to
add FSNOTIFY_EVENT_FROM_DENTRY, turning itself into ..._INODE and getting
d_name, even if you don't go for splitting that sucker.
	In any case, that's a separate patch and it might not be an
improvement.
	* inotify should_send_event - no need to bother with refcounting,
AFAICS, since the decision can be made before dropping i_lock.  BTW,
bool x; ..... x = foo ? true : false;  is spelled x = foo.  That's what
conversion to _Bool does, by definition, and kernel-side bool *is* _Bool.
	* inotify_handle_event() - um... why will ->wd we'd got from the
event we'd found and dropped survive through a bunch of blocking allocations?
With no locks held...
	* #10 and #11 might be worth merging.  Sure, you have them prior to
inotify conversion, but...
	* the stuff added in #9 looks odd.  Your queue is a FIFO; what happens
if I run into overflow, add overflow event into the end, remove some, drive
q_len down to something tolerable, then add more, then run into overflow again?
AFAICS, you get the same event queued twice for the same group, in the same
queue, with different priv.  Then you get ->free_event_priv() called on
flush_notify() for it.  Granted, that'll happen twice, so natural use of
get_priv_from_event() in the method instance will allow to DTRT, but that's
	a) asking for trouble
	b) at least needs to be commented.
Why not pass the damn pointer to priv to the method instead?  I'm not sure
where you are going with that stuff, but it would seem to make more sense...
AFAICS, the only current user (inotify) is OK *and* you are probably going
to add the next user yourself, so it can be changed later, but...

Overall: the only serious question I have right now is about ->wd in
inotify_handle_event().  Modulo that it's OK for -next; modulo that
and testing for regressions (*including* overhead ones)... I can live
with that going into mainline, provided that you are going to maintain
fs/notify/ afterwards.
--
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/