Elevated i_writecount doesn't guarantee ->release to be called

Elevated i_writecount doesn't guarantee ->release to be called

[Jan Kara]

  Changed subject and added linux-fsdevel to CC so that other developers
read this don't fall into the same trap :).

On Wed 28-01-15 22:45:34, Al Viro wrote:
> On Wed, Jan 28, 2015 at 01:45:24PM -0800, akpm@linux-foundation.org wrote:
> > atomic_t i_opencnt was used to free allocation in case there were no more
> > opens.  This patch replaces affs_file_open by generic_file_open and uses
> > FMODE_WRITE/i_writecount==1 for the task like other FS.
> 
> 
> >  affs_file_release(struct inode *inode, struct file *filp)
> >  {
> > -	pr_debug("release(%lu, %d)\n",
> > -		 inode->i_ino, atomic_read(&AFFS_I(inode)->i_opencnt));
> > +	pr_debug("release(%lu)\n", inode->i_ino);
> >  
> > -	if (atomic_dec_and_test(&AFFS_I(inode)->i_opencnt)) {
> > +	if ((filp->f_mode & FMODE_WRITE) &&
> > +	    (atomic_read(&inode->i_writecount) == 1)) {
> 
> I'm not at all convinced that this is correct for affs.  Or for anything
> else, for that matter.  Look: suppose somebody else is trying to open
> that sucker with O_TRUNC at that moment and they'd already gotten past
> get_write_access() in handle_truncate(), only to fail on locks_verify_locked().
> _That_ open() won't get anywhere near opening the file, so there won't be
> ->release() for it.  And our ->release() will see ->i_writecount greater
> than 1, due to get_write_access() done in handle_truncate() and still not
> balanced by coming put_write_access() in there - we'll call it after the
> locks_verify_locked() reports failure, but that hasn't happened yet.
> 
> Similar scenarios can almost certainly be constructed for other calls of
> get_write_access() as well, but this one is enough to NAK this patch, _and_
> to make the similar logics in other filesystems very suspicious...
  Thanks for pointing this out. You made me at look where exactly is
get_write_access() called and there are even places where we call it
without having file descriptor at all (e.g.  truncate path). So ext3, ext4,
udf, and gfs2 are racy. If we race, results aren't that bad (we just keep
preallocated blocks in the inode) but still it would be nice to fix.

Obviously we could maintain a private writecount in ->open() method but it
would seem a bit sad to do that for this mostly theoretical issue. Maybe we
just verify whether preallocation is truncated when evicting inode from
memory and if not, do it there. It's not perfect but even with current racy
solution noone noticed in practice.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Elevated i_writecount doesn't guarantee ->release to be called

[Fabian Frederick]

> On 29 January 2015 at 13:46 Jan Kara <jack@suse.cz> wrote:
>
>
>   Changed subject and added linux-fsdevel to CC so that other developers
> read this don't fall into the same trap :).
>
> On Wed 28-01-15 22:45:34, Al Viro wrote:
> > On Wed, Jan 28, 2015 at 01:45:24PM -0800, akpm@linux-foundation.org wrote:
> > > atomic_t i_opencnt was used to free allocation in case there were no more
> > > opens.  This patch replaces affs_file_open by generic_file_open and uses
> > > FMODE_WRITE/i_writecount==1 for the task like other FS.
> >
> >
> > >  affs_file_release(struct inode *inode, struct file *filp)
> > >  {
> > > - pr_debug("release(%lu, %d)\n",
> > > -          inode->i_ino, atomic_read(&AFFS_I(inode)->i_opencnt));
> > > + pr_debug("release(%lu)\n", inode->i_ino);
> > > 
> > > - if (atomic_dec_and_test(&AFFS_I(inode)->i_opencnt)) {
> > > + if ((filp->f_mode & FMODE_WRITE) &&
> > > +     (atomic_read(&inode->i_writecount) == 1)) {
> >
> > I'm not at all convinced that this is correct for affs.  Or for anything
> > else, for that matter.  Look: suppose somebody else is trying to open
> > that sucker with O_TRUNC at that moment and they'd already gotten past
> > get_write_access() in handle_truncate(), only to fail on
> > locks_verify_locked().
> > _That_ open() won't get anywhere near opening the file, so there won't be
> > ->release() for it.  And our ->release() will see ->i_writecount greater
> > than 1, due to get_write_access() done in handle_truncate() and still not
> > balanced by coming put_write_access() in there - we'll call it after the
> > locks_verify_locked() reports failure, but that hasn't happened yet.
> >
> > Similar scenarios can almost certainly be constructed for other calls of
> > get_write_access() as well, but this one is enough to NAK this patch, _and_
> > to make the similar logics in other filesystems very suspicious...
>   Thanks for pointing this out. You made me at look where exactly is
> get_write_access() called and there are even places where we call it
> without having file descriptor at all (e.g.  truncate path). So ext3, ext4,
> udf, and gfs2 are racy. If we race, results aren't that bad (we just keep
> preallocated blocks in the inode) but still it would be nice to fix.
>
> Obviously we could maintain a private writecount in ->open() method but it
> would seem a bit sad to do that for this mostly theoretical issue. Maybe we
> just verify whether preallocation is truncated when evicting inode from
> memory and if not, do it there. It's not perfect but even with current racy
> solution noone noticed in practice.
Note that udf is slightly different ; it checks for i_writecount > 1 not =1
which means it would release the file in scenario described above ...

Regards,
Fabian

>
>                                                               Honza
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Elevated i_writecount doesn't guarantee ->release to be called

[Jan Kara]

On Thu 29-01-15 17:47:56, Fabian Frederick wrote:
> > On 29 January 2015 at 13:46 Jan Kara <jack@suse.cz> wrote:
> >
> >
> >   Changed subject and added linux-fsdevel to CC so that other developers
> > read this don't fall into the same trap :).
> >
> > On Wed 28-01-15 22:45:34, Al Viro wrote:
> > > On Wed, Jan 28, 2015 at 01:45:24PM -0800, akpm@linux-foundation.org wrote:
> > > > atomic_t i_opencnt was used to free allocation in case there were no more
> > > > opens.  This patch replaces affs_file_open by generic_file_open and uses
> > > > FMODE_WRITE/i_writecount==1 for the task like other FS.
> > >
> > >
> > > >  affs_file_release(struct inode *inode, struct file *filp)
> > > >  {
> > > > - pr_debug("release(%lu, %d)\n",
> > > > -          inode->i_ino, atomic_read(&AFFS_I(inode)->i_opencnt));
> > > > + pr_debug("release(%lu)\n", inode->i_ino);
> > > > 
> > > > - if (atomic_dec_and_test(&AFFS_I(inode)->i_opencnt)) {
> > > > + if ((filp->f_mode & FMODE_WRITE) &&
> > > > +     (atomic_read(&inode->i_writecount) == 1)) {
> > >
> > > I'm not at all convinced that this is correct for affs.  Or for anything
> > > else, for that matter.  Look: suppose somebody else is trying to open
> > > that sucker with O_TRUNC at that moment and they'd already gotten past
> > > get_write_access() in handle_truncate(), only to fail on
> > > locks_verify_locked().
> > > _That_ open() won't get anywhere near opening the file, so there won't be
> > > ->release() for it.  And our ->release() will see ->i_writecount greater
> > > than 1, due to get_write_access() done in handle_truncate() and still not
> > > balanced by coming put_write_access() in there - we'll call it after the
> > > locks_verify_locked() reports failure, but that hasn't happened yet.
> > >
> > > Similar scenarios can almost certainly be constructed for other calls of
> > > get_write_access() as well, but this one is enough to NAK this patch, _and_
> > > to make the similar logics in other filesystems very suspicious...
> >   Thanks for pointing this out. You made me at look where exactly is
> > get_write_access() called and there are even places where we call it
> > without having file descriptor at all (e.g.  truncate path). So ext3, ext4,
> > udf, and gfs2 are racy. If we race, results aren't that bad (we just keep
> > preallocated blocks in the inode) but still it would be nice to fix.
> >
> > Obviously we could maintain a private writecount in ->open() method but it
> > would seem a bit sad to do that for this mostly theoretical issue. Maybe we
> > just verify whether preallocation is truncated when evicting inode from
> > memory and if not, do it there. It's not perfect but even with current racy
> > solution noone noticed in practice.
> Note that udf is slightly different ; it checks for i_writecount > 1 not =1
> which means it would release the file in scenario described above ...
  I know but that's because it has two bugs in a single condition ;) I have
a patch queued for udf which changes the condition to == 1.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Elevated i_writecount doesn't guarantee ->release to be called

[Al Viro]

On Thu, Jan 29, 2015 at 01:46:30PM +0100, Jan Kara wrote:
>   Thanks for pointing this out. You made me at look where exactly is
> get_write_access() called and there are even places where we call it
> without having file descriptor at all (e.g.  truncate path). So ext3, ext4,
> udf, and gfs2 are racy. If we race, results aren't that bad (we just keep
> preallocated blocks in the inode) but still it would be nice to fix.
> 
> Obviously we could maintain a private writecount in ->open() method but it
> would seem a bit sad to do that for this mostly theoretical issue. Maybe we
> just verify whether preallocation is truncated when evicting inode from
> memory and if not, do it there. It's not perfect but even with current racy
> solution noone noticed in practice.

The trouble with doing that on inode eviction is that we might have done
r/o remount by then, so any metadata writes are unexpected at that point...

Re: Elevated i_writecount doesn't guarantee ->release to be called

[Fabian Frederick]

> On 29 January 2015 at 18:35 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>
> On Thu, Jan 29, 2015 at 01:46:30PM +0100, Jan Kara wrote:
> >   Thanks for pointing this out. You made me at look where exactly is
> > get_write_access() called and there are even places where we call it
> > without having file descriptor at all (e.g.  truncate path). So ext3, ext4,
> > udf, and gfs2 are racy. If we race, results aren't that bad (we just keep
> > preallocated blocks in the inode) but still it would be nice to fix.
> >
> > Obviously we could maintain a private writecount in ->open() method but it
> > would seem a bit sad to do that for this mostly theoretical issue. Maybe we
> > just verify whether preallocation is truncated when evicting inode from
> > memory and if not, do it there. It's not perfect but even with current racy
> > solution noone noticed in practice.
>
> The trouble with doing that on inode eviction is that we might have done
> r/o remount by then, so any metadata writes are unexpected at that point...

What would you suggest then ? I could try to do the right update on AFFS
then apply it on ext3,4,udf,gfs2 as well. Maybe some generic_release
would be useful to avoid repeating the same logic around private
preallocation management ?

Regards,
Fabian
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html