RE: [PATCH] ksmbd: fix memory leaks and NULL deref in smb2_lock()

[Werner Kasselman]

Thanks ChenXiaoSong,

Both suggestions incorporated in v2:

1. Hoisted locks_free_lock()/kfree() before the if(!rc) check in the UNLOCK branch, all exit paths now share one free site instead of duplicating it in the -ENOENT branch.
2. Replaced the duplicated rollback cleanup with an if(rlock) guard around the VFS unlock and a matching NULL check on locks_free_lock(rlock) in the shared cleanup tail.  This eliminates the code duplication we had in v1 where the !rlock path copied the entire teardown sequence.

v2 sent.

Kind regards,
Werner

-----Original Message-----
From: ChenXiaoSong <chenxiaosong@xxxxxxxxxxxxxxxx> 
Sent: Tuesday, 17 March 2026 7:30 PM
To: Werner Kasselman <werner@xxxxxxxxxx>; Namjae Jeon <linkinjeon@xxxxxxxxxx>; Steve French <smfrench@xxxxxxxxx>
Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>; Tom Talpey <tom@xxxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] ksmbd: fix memory leaks and NULL deref in smb2_lock()

And it might be better to change it as follows.

```
@@ -7685,13 +7686,17 @@ int smb2_lock(struct ksmbd_work *work)
                 struct file_lock *rlock = NULL;

                 rlock = smb_flock_init(filp);
-               rlock->c.flc_type = F_UNLCK;
-               rlock->fl_start = smb_lock->start;
-               rlock->fl_end = smb_lock->end;
+               if (rlock) {
+                       rlock->c.flc_type = F_UNLCK;
+                       rlock->fl_start = smb_lock->start;
+                       rlock->fl_end = smb_lock->end;

-               rc = vfs_lock_file(filp, F_SETLK, rlock, NULL);
-               if (rc)
-                       pr_err("rollback unlock fail : %d\n", rc);
+                       rc = vfs_lock_file(filp, F_SETLK, rlock, NULL);
+                       if (rc)
+                               pr_err("rollback unlock fail : %d\n", rc);
+               } else {
+                       pr_err("rollback unlock alloc failed\n");
+               }

                 list_del(&smb_lock->llist);
                 spin_lock(&work->conn->llist_lock);
@@ -7701,7 +7706,8 @@ int smb2_lock(struct ksmbd_work *work)
                 spin_unlock(&work->conn->llist_lock);

                 locks_free_lock(smb_lock->fl);
-               locks_free_lock(rlock);
+               if (rlock)
+                       locks_free_lock(rlock);
                 kfree(smb_lock);
         }
  out2:
```

Thanks,
ChenXiaoSong <chenxiaosong@xxxxxxxxxx>

On 3/17/26 16:08, Werner Kasselman wrote:
> @@ -7685,6 +7691,19 @@ int smb2_lock(struct ksmbd_work *work)
>   		struct file_lock *rlock = NULL;
>   
>   		rlock = smb_flock_init(filp);
> +		if (!rlock) {
> +			pr_err("rollback unlock alloc failed\n");
> +			list_del(&smb_lock->llist);
> +			spin_lock(&work->conn->llist_lock);
> +			if (!list_empty(&smb_lock->flist))
> +				list_del(&smb_lock->flist);
> +			list_del(&smb_lock->clist);
> +			spin_unlock(&work->conn->llist_lock);
> +
> +			locks_free_lock(smb_lock->fl);
> +			kfree(smb_lock);
> +			continue;
> +		}
>   		rlock->c.flc_type = F_UNLCK;
>   		rlock->fl_start = smb_lock->start;
>   		rlock->fl_end = smb_lock->end;