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.
