On Mon, Mar 22, 2021 at 02:13:42PM +0900, Namjae Jeon wrote:

quoted

This adds file operations and buffer pool for cifsd.

Some random notes:

quoted

+static void rollback_path_modification(char *filename) {
+	if (filename) {
+		filename--;
+		*filename = '/';

What an odd way to spell filename[-1] = '/';...

Okay. Will fix.

quoted

+int ksmbd_vfs_inode_permission(struct dentry *dentry, int acc_mode,
+bool delete) {

quoted

+	if (delete) {
+		struct dentry *parent;
+
+		parent = dget_parent(dentry);
+		if (!parent)
+			return -EINVAL;
+
+		if (inode_permission(&init_user_ns, d_inode(parent), MAY_EXEC | MAY_WRITE)) {
+			dput(parent);
+			return -EACCES;
+		}
+		dput(parent);

Who's to guarantee that parent is stable?  IOW, by the time of that
inode_permission() call dentry might very well not be a child of that thing...

Okay, Will fix.

quoted

+	parent = dget_parent(dentry);
+	if (!parent)
+		return 0;
+
+	if (!inode_permission(&init_user_ns, d_inode(parent), MAY_EXEC | MAY_WRITE))
+		*daccess |= FILE_DELETE_LE;

Ditto.

Okay.

quoted

+int ksmbd_vfs_mkdir(struct ksmbd_work *work,
+		    const char *name,
+		    umode_t mode)

quoted

+	err = vfs_mkdir(&init_user_ns, d_inode(path.dentry), dentry, mode);
+	if (!err) {
+		ksmbd_vfs_inherit_owner(work, d_inode(path.dentry),
+			d_inode(dentry));

->mkdir() might very well return success, with dentry left unhashed negative.
Look at the callers of vfs_mkdir() to see how it should be handled.

Okay, Will fix.

quoted

+static int check_lock_range(struct file *filp,
+			    loff_t start,
+			    loff_t end,
+			    unsigned char type)
+{
+	struct file_lock *flock;
+	struct file_lock_context *ctx = file_inode(filp)->i_flctx;
+	int error = 0;
+
+	if (!ctx || list_empty_careful(&ctx->flc_posix))
+		return 0;
+
+	spin_lock(&ctx->flc_lock);
+	list_for_each_entry(flock, &ctx->flc_posix, fl_list) {
+		/* check conflict locks */
+		if (flock->fl_end >= start && end >= flock->fl_start) {
+			if (flock->fl_type == F_RDLCK) {
+				if (type == WRITE) {
+					ksmbd_err("not allow write by shared lock\n");
+					error = 1;
+					goto out;
+				}
+			} else if (flock->fl_type == F_WRLCK) {
+				/* check owner in lock */
+				if (flock->fl_file != filp) {
+					error = 1;
+					ksmbd_err("not allow rw access by exclusive lock from other

opens\n");

quoted

+					goto out;
+				}
+			}
+		}
+	}
+out:
+	spin_unlock(&ctx->flc_lock);
+	return error;
+}

WTF is that doing in smbd?

This code was added to pass the smb2 lock test of samba's smbtorture.
Will fix it.

quoted

+	filp = fp->filp;
+	inode = d_inode(filp->f_path.dentry);

That should be file_inode().  Try it on overlayfs, watch it do interesting things...

Okay.

quoted

+	nbytes = kernel_read(filp, rbuf, count, pos);
+	if (nbytes < 0) {
+		name = d_path(&filp->f_path, namebuf, sizeof(namebuf));
+		if (IS_ERR(name))
+			name = "(error)";
+		ksmbd_err("smb read failed for (%s), err = %zd\n",
+				name, nbytes);

Do you really want the full pathname here?  For (presumably) spew into syslog?

No, Will fix.

quoted

+int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name) {
+	struct path parent;
+	struct dentry *dir, *dentry;
+	char *last;
+	int err = -ENOENT;
+
+	last = extract_last_component(name);
+	if (!last)
+		return -ENOENT;

Yeccchhh...

I guess I change it err instead of -ENOENT.

quoted

+	if (ksmbd_override_fsids(work))
+		return -ENOMEM;
+
+	err = kern_path(name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &parent);
+	if (err) {
+		ksmbd_debug(VFS, "can't get %s, err %d\n", name, err);
+		ksmbd_revert_fsids(work);
+		rollback_path_modification(last);
+		return err;
+	}
+
+	dir = parent.dentry;
+	if (!d_inode(dir))
+		goto out;

Really?  When does that happen?

Will fix.

quoted

+static int __ksmbd_vfs_rename(struct ksmbd_work *work,
+			      struct dentry *src_dent_parent,
+			      struct dentry *src_dent,
+			      struct dentry *dst_dent_parent,
+			      struct dentry *trap_dent,
+			      char *dst_name)
+{
+	struct dentry *dst_dent;
+	int err;
+
+	spin_lock(&src_dent->d_lock);
+	list_for_each_entry(dst_dent, &src_dent->d_subdirs, d_child) {
+		struct ksmbd_file *child_fp;
+
+		if (d_really_is_negative(dst_dent))
+			continue;
+
+		child_fp = ksmbd_lookup_fd_inode(d_inode(dst_dent));
+		if (child_fp) {
+			spin_unlock(&src_dent->d_lock);
+			ksmbd_debug(VFS, "Forbid rename, sub file/dir is in use\n");
+			return -EACCES;
+		}
+	}
+	spin_unlock(&src_dent->d_lock);

Hard NAK right there.  That thing has no business poking at that level.
And I'm pretty certain that it's racy as hell.

Okay. It was same reason(smbtorture test), will fix it also.

Thanks for your review!