Re: [PATCH 3/5] cifsd: add file operations

[Al Viro <viro@zeniv.linux.org.uk>]

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.