Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes: > On Wed, Dec 14, 2011 at 06:15:11PM -0800, Linus Torvalds wrote: >> Looks reasonable, but why doesn't all callers have that "put_mark()" thing? >> >> And if/when all callers *do* have that put_mark() thing, maybe we >> should make destroy_mark() just do it? >> >> In particular, a quick grep shows that there are destroy_mark users still in: >> >> - fs/notify/fanotify/fanotify_user.c >> >> - fs/notify/dnotify/dnotify.c (2 of them) >> >> - fs/notify/inotify/inotify_fsnotify.c >> >> >> that don't do "put_mark()" after the destroy. Why is it ok there? > > Um? dnotify has fsnotify_put_mark() called in both cases... > >> I don't know the code, it's probably fine, but I'd like to know why >> the audit code needs it but not the other sites (but my grep didn't >> look at context) >> >> And I'd like Al to say something. Al? > > I don't like it; it's called from ->handle_event() and parent->mark is > exactly the inode_mark argument of that method. It ought to be pinned > by caller. In other places we *do* need get/put around that destroy > and we generally do that. > > AFAICS, we have the following picture: > * that place in audit_watch - argument of ->handle_event() > * audit_remove_watch_rule() - pinned explicitly > * audit_tree - pinned explicitly > * dnotify (both callrs) - pinned explicitly, and refcount is > dropped unconditionally while fsnotify_destroy_mark() is *not*; IOW, > that's a very strong argument against folding put_mark into destroy_mark. > * inotify_fsnotify.c - argument of ->handle_event() > * fanotify_user.c - pinned and dropped by caller; again, refcount > manipulations are unconditional while destroy_mark is not; it's even > worse than in dnotify case, since here we do put_mark is a place where > we don't *know* whether destroy_mark has happened. We could move the > calls of fsnotify_put_mark() into the fanotify_mark_remove_from_mask() > (where destroy_mark is done), but then we'll get something like > if (!(oldmask & ~mask)) > fsnotify_destroy_mark(fsn_mark); > else > fsnotify_put_mark(fsn_mark); > in there, which is IMO ugly. > > Guys, does anybody have a real demonstration of the breakage cured by > pinning the mark down in audit_watch.c ->handle_event()? Or is that > a pure theory? Yes it does fix the BUG. Test case in patch. > Is ->handle_event() argument held by caller? Well, obviously not, otherwise we wouldn't hit the bug. > Eric? If that's the case, > we don't need to do anything with audit_watch.c instance; otherwise, > both that one and inotify_handle_event() are in trouble... Yep. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
