> On Nov 7, 2022, at 5:48 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Sun, 2022-11-06 at 14:02 -0500, trondmy@xxxxxxxxxx wrote:
>> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>
>> vfs_lock_file() expects the struct file_lock to be fully initialised by
>> the caller. Re-exported NFSv3 has been seen to Oops if the fl_file field
>> is NULL.
>>
>> Fixes: aec158242b87 ("lockd: set fl_owner when unlocking files")
>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>> ---
>> fs/lockd/svcsubs.c | 17 ++++++++++-------
>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
>> index e1c4617de771..3515f17eaf3f 100644
>> --- a/fs/lockd/svcsubs.c
>> +++ b/fs/lockd/svcsubs.c
>> @@ -176,7 +176,7 @@ nlm_delete_file(struct nlm_file *file)
>> }
>> }
>>
>> -static int nlm_unlock_files(struct nlm_file *file, fl_owner_t owner)
>> +static int nlm_unlock_files(struct nlm_file *file, const struct file_lock *fl)
>> {
>> struct file_lock lock;
>>
>> @@ -184,12 +184,15 @@ static int nlm_unlock_files(struct nlm_file *file, fl_owner_t owner)
>> lock.fl_type = F_UNLCK;
>> lock.fl_start = 0;
>> lock.fl_end = OFFSET_MAX;
>> - lock.fl_owner = owner;
>> - if (file->f_file[O_RDONLY] &&
>> - vfs_lock_file(file->f_file[O_RDONLY], F_SETLK, &lock, NULL))
>> + lock.fl_owner = fl->fl_owner;
>> + lock.fl_pid = fl->fl_pid;
>> + lock.fl_flags = FL_POSIX;
>> +
>> + lock.fl_file = file->f_file[O_RDONLY];
>> + if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, NULL))
>> goto out_err;
>> - if (file->f_file[O_WRONLY] &&
>> - vfs_lock_file(file->f_file[O_WRONLY], F_SETLK, &lock, NULL))
>> + lock.fl_file = file->f_file[O_WRONLY];
>> + if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, NULL))
>> goto out_err;
>> return 0;
>> out_err:
>> @@ -226,7 +229,7 @@ nlm_traverse_locks(struct nlm_host *host, struct nlm_file *file,
>> if (match(lockhost, host)) {
>>
>> spin_unlock(&flctx->flc_lock);
>> - if (nlm_unlock_files(file, fl->fl_owner))
>> + if (nlm_unlock_files(file, fl))
>> return 1;
>> goto again;
>> }
>
> Good catch.
>
> I wonder if we ought to roll an initializer function for file_locks to
> make it harder for callers to miss setting some fields like this? One
> idea: we could change vfs_lock_file to *not* take a file argument, and
> insist that the caller fill out fl_file when calling it? That would make
> it harder to screw this up.
>
> In any case, let's take this patch in the interim while we consider
> whether and how to clean this up.
>
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
Since this doesn't fix breakage in 6.1-rc, I plan to take it for 6.2.
If all y'all feel the fix is more urgent than that, let me know.
--
Chuck Lever
