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

This adds file operations and buffer pool for cifsd.

Some random notes:

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

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

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

+	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...

+	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.

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

+	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.

+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");
+					goto out;
+				}
+			}
+		}
+	}
+out:
+	spin_unlock(&ctx->flc_lock);
+	return error;
+}

WTF is that doing in smbd?

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

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

+	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?

+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...

+	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?

+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.