The file_lock_operatoins.lock API seems to be a BAD API.

The file_lock_operatoins.lock API seems to be a BAD API.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]


How many times does an API need to be misused before we throw it out?

When a file is closed, locks_remove_posix() is called and this
*must* remove all locks.  The 'struct file' will be removed, so
->fl_file will become a dangling link.
But the 'struct file_lock' will still be on the global lock list
and will trigger and oops if anyone reads /proc/locks.

However, locks_remove_posix() will call filp->f_op->lock() if it exists,
and there are one or two (or more) filesystems which seem to think that
it is OK for unlock to fail due to a signal or network error etc.

It isn't that long ago since this sort of problem was fixed in cifs and
cephfs (well... it isn't that long since the fix was backported to a SLE
kernel and I noticed - I don't recall when the fix hit mainline).
It was (partly) fixed in NFS a few years back (f30cb757f) - I just
tripped over this again for an SLE-LTS kernel.

Just this week I've seen the NFS bug and and also a gpfs (out-of-tree
GPL code) bug.
I just wandered through the current code and found ....

orangefs:  This won't remove locks if ORANGEFS_OPT_LOCK_LOCK is not
  set.  It won't add them either so maybe that is OK .... except that I
  think "mount -o remount" can clear that flag.  If you clear it while a
  lock is held - badness results.

fuse - if fc->no_lock is zero - seems to assume that sending a
   request to the daemon - fuse_simple_request() - *will* be
   sufficient.  I wouldn't trust any daemon not to trigger an oops.
   It also assumes that "locking is restartable" - (ok to return
   ERESTARTSYS), but 'locks_remove_posix()' isn't.

gfs2, oscfs both use dlk_posix_lock which will fail if kmalloc() fails
  (which is certainly possible if the close is happening because the
  process was killed by the OOM killer).  I'm not sure if there are
  other failure modes.

NFSv2 and NFSv3 use nlmclnt_proc which can also fail on kmalloc failure.

NFSv4 can nominally fail if 'ctx->state' is NULL - but that is probably
    impossible.  A WARN() might no go astray there.

NFS will allow do_unlk() to fail if a fatal signal is pending, but not
in the FL_CLOSE case, so that protects locks_remove_posix() and
locks_remove_flock().  Maybe other calls to unlock don't matter ... if
there is a fatal signal pending they definitely don't - which makes the
speical-casing of FL_CLOSE rather pointless.

locks_remove_posix() and locks_remove_flock() aren't the only problem
areas.  nfsd will call vfs_setlease(.. F_UNLCK..) and assume that the
lease is gone.  If the filesystem messes up and doesn't remove the
lease, then when the lease subsequently gets broken, it will access some
nfsd data structure that has been freed.  Oops.

I don't think we should just fix all those bugs in those filesystems.
I think that F_UNLCK should *always* remove the lock/lease.
I imaging this happening by  *always* calling posix_lock_file() (or
similar) in the unlock case - after calling f_op->lock() first if that
is appropriate.

What do people think?  It there on obvious reason that is a non-starter?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: The file_lock_operatoins.lock API seems to be a BAD API.

[J. Bruce Fields]

On Thu, May 28, 2020 at 04:14:44PM +1000, NeilBrown wrote:
> I don't think we should just fix all those bugs in those filesystems.
> I think that F_UNLCK should *always* remove the lock/lease.
> I imaging this happening by  *always* calling posix_lock_file() (or
> similar) in the unlock case - after calling f_op->lock() first if that
> is appropriate.
> 
> What do people think?  It there on obvious reason that is a non-starter?

Isn't NFS unlock like close, in that it may be our only chance to return
IO errors?

But I guess you're not saying that unlock can't return errors, just that
it should always remove the lock whether it returns 0 or not.

Hm.

--b.

Re: The file_lock_operatoins.lock API seems to be a BAD API.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

On Thu, May 28 2020, J. Bruce Fields wrote:

> On Thu, May 28, 2020 at 04:14:44PM +1000, NeilBrown wrote:
>> I don't think we should just fix all those bugs in those filesystems.
>> I think that F_UNLCK should *always* remove the lock/lease.
>> I imaging this happening by  *always* calling posix_lock_file() (or
>> similar) in the unlock case - after calling f_op->lock() first if that
>> is appropriate.
>> 
>> What do people think?  It there on obvious reason that is a non-starter?
>
> Isn't NFS unlock like close, in that it may be our only chance to return
> IO errors?

Is it?  fcntl() isn't documented as returning ENOSPC, EDQUOT or EIO.

>
> But I guess you're not saying that unlock can't return errors, just that
> it should always remove the lock whether it returns 0 or not.

No I wasn't, but I might.
One approach that I was considering for making the API more robust was
to propose a separate "unlock" file_operation.  All unlock requests
would go through this, and it would have a 'void' return type.
Would that be sufficient to encourage programmers to handle their own
errors and not think they can punt?

But yes - even if unlock returns an error, it should (locally) remove
any locks - much as 'close()' will always close the fd (if it was
actually open) even if it reports an error.

Thanks,
NeilBrown

>
> Hm.
>
> --b.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: The file_lock_operatoins.lock API seems to be a BAD API.

[J. Bruce Fields]

On Fri, May 29, 2020 at 11:01:59AM +1000, NeilBrown wrote:
> On Thu, May 28 2020, J. Bruce Fields wrote:
> 
> > On Thu, May 28, 2020 at 04:14:44PM +1000, NeilBrown wrote:
> >> I don't think we should just fix all those bugs in those filesystems.
> >> I think that F_UNLCK should *always* remove the lock/lease.
> >> I imaging this happening by  *always* calling posix_lock_file() (or
> >> similar) in the unlock case - after calling f_op->lock() first if that
> >> is appropriate.
> >> 
> >> What do people think?  It there on obvious reason that is a non-starter?
> >
> > Isn't NFS unlock like close, in that it may be our only chance to return
> > IO errors?
> 
> Is it?  fcntl() isn't documented as returning ENOSPC, EDQUOT or EIO.

I'm probably wrong.  Writes have to be acknowledged before we return
from unlock, but that doesn't mean that's our only chance to return any
errors we find at that point.

> > But I guess you're not saying that unlock can't return errors, just that
> > it should always remove the lock whether it returns 0 or not.
> 
> No I wasn't, but I might.
> One approach that I was considering for making the API more robust was
> to propose a separate "unlock" file_operation.  All unlock requests
> would go through this, and it would have a 'void' return type.
> Would that be sufficient to encourage programmers to handle their own
> errors and not think they can punt?

Maybe I regret bringing up errors....  As you say, the important thing
is making sure the lock's cleaned up, and doing that in common code
sounds like the way to guarantee it.

> But yes - even if unlock returns an error, it should (locally) remove
> any locks - much as 'close()' will always close the fd (if it was
> actually open) even if it reports an error.

That makes sense to me.

--b.