Re: [PATCH 1/5] fsnotify: grab inode ref in add_inode_mark() instead of add_mark()

[Eric Paris <eparis@redhat.com>]

On Tue, 2011-03-01 at 19:30 +0100, Lino Sanfilippo wrote:
> On Fri, Feb 25, 2011 at 01:36:35PM -0500, Eric Paris wrote:
> > I think your patch series is making a noticeable change which I don't
> > think I'm comfortable with.  The current code does not pin inodes in
> > core if they only have ignored masks.  Under memory pressure those
> > inodes can get eviced and the mark will get cleaned up.  My glance at
> > this code leads me to believe that all inodes with any mark is going to
> > be pinned in core.  I don't think that's a good idea for AV vendor use
> > where they might want to watch everything on the system with mount point
> > marks and put ignored marks on everything that comes along to cache
> > allows.
> > 
> > The fact that inodes might not be pinned in core is the reason for some
> > of the stupid difficulty.  There is probably some way to work it out but
> > I think it's something I'm going to need to think about.
> 
> Youre right, the inode is now also pinned if there is no mask set. This is
> a change i did not on purpose. Whether the inode is pinned or not does not 
> affect the original purpose of the patch series, which was the decoupling 
> of the mark lock and the fsobject lock. I simply forgot to check the mask.
> I could replace that part with something like
> 
> -       mark->i.inode = igrab(inode);
> -       mark->flags |= FSNOTIFY_MARK_FLAG_OBJECT_PINNED;
> +       mark->i.inode = inode;
> +       if (mark->mask) {       /* only pin if mask is set */   
> +               igrab(inode);
> +               mark->flags |= FSNOTIFY_MARK_FLAG_OBJECT_PINNED;
> +       }
> 
> Should i just fix it and resend the patches? Or do you have any other
> concerns?

No, but it's a rather serious concern to me.  You need to make sure that
mark->i.inode is actually valid before you use it and cannot disappear
underneath you.  I haven't relooked at your code (and clearly will after
you fix) but what I'm most concerned about is some place where we are
trying to delete a mark, either explicitly or by group, and then race
with the inode being evicted from cache.  The inode being evicted from
cache is going to eventually hit the destroy_by_inode code.  When that
function returns we cannot use mark->i.inode any more.  In today's code
we prevent this situation by never dereferencing or using the
mark->i.inode outside of mark->lock.  You're going to have to remember
that after a 'destroy_by_inode' call you can't ever use the inode
again....

-Eric