ksmbd_vfs_kern_path_locked behaviour

[mmakassikis]

Hello,

While looking at building the SMB1 code on a recent kernel, I saw that ksmbd_vfs_kern_path has been replaced with ksmbd_vfs_kern_path_locked (torvalds/linux@74d7970febf)

If I understand the usage of the _locked version, the caller must unlock the parent inode at the same time path_put() is called.

Is that correct ? There are quite a few places in smb1pdu.c where ksmbd_vfs_kern_path is currently locked, so that would change the cleanup code in all these call sites.

In addition, I don't understand this change in smb2_open():

    if (file_present || created) {
        inode_unlock(d_inode(path.dentry->d_parent));
        dput(path.dentry);
    }

Why is it dput(path.dentry) instead of path_put(&path) ? path_put() does dput() followed by mntput(). Is there a reason why we don't need the mntput() call ?

Regards, Marios

[namjaejeon]

@mmakassikis

ksmbd_vfs_kern_path_locked create path and store ->mnt using mnt of share_conf.

    path->dentry = d;
    path->mnt = share_conf->vfs_path.mnt;

path_put will release both ->dentry and ->mnt of path struct. but ->mnt is mnt of share_conf. So It should be not release before umount(tree disconnect). And It will be changed to path_put from dput on "ksmbd: check if a mount point is crossed during path lookup" patch.

[namjaejeon]

@mmakassikis

If We want to call path_put instead of dput for this patch. We need to add mntget to increment reference count of mnt.

path->dentry = d;
path->mnt = mntget(share_conf->vfs_path.mnt); <-- mntget()

[mmakassikis]

@namjaejeon Thanks for the explanation. I somehow missed the path->mnt assignment.

I'm patching smb1pdu.c to use ksmbd_vfs_kern_path_locked. The conversion is relatively straightforward. For every call, a parent_path is needed as well, and when we're done using the struct path, the cleanup is as follows:

inode_unlock(d_inode(parent_path.dentry));
path_put(&path);
path_put(&parent_path);

Is this correct ?

There is one call site where I can't tell what the proper behaviour is. In find_first(), the path is path_put() in the error path, or in the "end of search" case. Otherwise, the dir_fp is kept around for use in find_next(), where it is released in the same circumstances (error or "end of search").

This raises 2 questions:

• in the current code, is it possible to miss a path_put() if a client connects, sends a "find first" request and then disconnects ?
• using ksmbd_vfs_kern_path_locked, when should the cleanup be done (path_put(&parent_path)and inode_unlock) ?

Since we're not doing any renaming/deleting here, I don't know if the inode_lock on the parent needs to kept for the lifetime of the looked up path or not.

[namjaejeon]

Is this correct ? Sorry, I didn't understand. I think there will be no problem if you bring the code from smb2_open.

the path is path_put() in the error path, or in the "end of search" case. Otherwise, the dir_fp is kept around for use in find_next(), where it is released in the same circumstances (error or "end of search").

each ksmbd_vfs_kern_path() and dentry_open call path_get. So we should call path_put at the end of find_first.

in the current code, is it possible to miss a path_put() if a client connects, sends a "find first" request and then disconnects ?

find_first -> ksmbd_vfs_kern_path(path_get increment reference count) -> dentry_open(call path_get again) -> surely call path_put at the end of find_first(). If end of search in first_first, need to call path_put two time. i.e. path_put(&path) and path_put(&(dir_fp->filp->f_path));

if find_next is called, It will call ksmbd_fd_put() which will call path_put().

using ksmbd_vfs_kern_path_locked, when should the cleanup be done (path_put(&parent_path)and inode_unlock) ?

files can be move to different directory using rename during find_XXX(). ksmbd_vfs_kern_path_locked will protect file change in parent directory. So I think that it should protect iterate_dir(). I didn't check parentinode is needed for iterate_dir(). I didn't check if iterate_dir can lock parent inode.

[mmakassikis]

Sorry, I didn't understand. I think there will be no problem if you bring the code from smb2_open.

Let me rephrase my question:

Previously, when using ksmbd_vfs_kern_path(), we needed to call path_put(&path) to decrement the reference count. Now, with ksmbd_vfs_kern_path_locked(), we increment the reference count on a path, a parent_path and obtain an inode lock. So when we are done using them, we need to decrement the reference counts, and release the lock (as in the previous code sample).

This is what I understand from the changes in smb2pdu.c, but I wanted to confirm to make sure I didn't miss anything.

Because SMB1 has many requests where a filename is given rather than a handle, there are more places where ksmbd_vfs_kern_path() is called, so the conversion takes a little time.

find_first -> ksmbd_vfs_kern_path(path_get increment reference count) -> dentry_open(call path_get again) -> surely call path_put at the end of find_first(). If end of search in first_first, need to call path_put two time. i.e. path_put(&path) and path_put(&(dir_fp->filp->f_path));

Ok, so path_put() is missing in find_first() in both error and normal codepaths. I have opened a pull request https://github.com/namjaejeon/ksmbd/pull/452 (though it may not build on recent kernels).

I didn't check if iterate_dir can lock parent inode.

I am not sure how/where to verify this. If you have any ideas please let me know.

[mmakassikis]

files can be move to different directory using rename during find_XXX(). ksmbd_vfs_kern_path_locked will protect file change in parent directory. So I think that it should protect iterate_dir(). I didn't check parentinode is needed for iterate_dir(). I didn't check if iterate_dir can lock parent inode.

I have a work-in-progress patch to convert smb_rename() to use ksmbd_vfs_kern_path_locked() and ksmbd_vfs_rename(). In order to make it work, I have to unlock the parent inode, because ksmbd_vfs_rename() will try to lock, which causes a deadlock (and a very unhappy lockdep).

If a client sends a find_first(), followed by a smb_rename() and the parent inode lock is held, then the thread is going to deadlock. There's no guarantee either that the client will send find_next() or that 'end of search' condition is reached. Therefore, I don't think we can hold the parent inode lock during find_first/first_next requests. Thoughts ?

[namjaejeon]

I have to unlock the parent inode, because ksmbd_vfs_rename() will try to lock, which causes a deadlock (and a very unhappy lockdep).

It will be no problem if you unlock parent inode before calling ksmbd_vfs_rename() in smb_rename().

Therefore, I don't think we can hold the parent inode lock during find_first/first_next requests. Thoughts ?

Yes. We can use ksmbd_vfs_kern_path_locked() till getting dir_fp(using ksmbd_vfs_dentry_open) and unlock parent lock and release parent path under ksmbd_vfs_dentry_open().

[mmakassikis]

Closing this as the changes have been committed (d60c3c849a3e)

Thanks for the help!

[namjaejeon]

@mmakassikis I don't test smb1 well. I'm sure you tested it well. It would be better if you verify it with xfstests. there is xfstests script in github action script. Thanks for your work!