Re: [PATCH v18] fs: Add VirtualBox guest shared folder (vboxsf) support

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

On Mon, Nov 25, 2019 at 03:08:39PM +0100, Hans de Goede wrote:

> +	list_for_each_entry(b, &sf_d->info_list, head) {
> +try_next_entry:
> +		if (ctx->pos >= cur + b->entries) {
> +			cur += b->entries;
> +			continue;
> +		}
> +
> +		/*
> +		 * Note the vboxsf_dir_info objects we are iterating over here
> +		 * are variable sized, so the info pointer may end up being
> +		 * unaligned. This is how we get the data from the host.
> +		 * Since vboxsf is only supported on x86 machines this is not
> +		 * a problem.
> +		 */
> +		for (i = 0, info = b->buf; i < ctx->pos - cur; i++) {
> +			size = offsetof(struct shfl_dirinfo, name.string) +
> +			       info->name.size;
> +			info = (struct shfl_dirinfo *)((uintptr_t)info + size);

Yecchhh...
	1) end = &info->name.string[info->name.size];
	   info = (struct shfl_dirinfo *)end;
please.  Compiler can and will optimize it just fine.
	2) what guarantees the lack of overruns here?

> +{
> +	bool keep_iterating;
> +
> +	for (keep_iterating = true; keep_iterating; ctx->pos += 1)
> +		keep_iterating = vboxsf_dir_emit(dir, ctx);

Are you sure you want to bump ctx->pos when vboxsf_dir_emit() returns false?

> +static int vboxsf_dir_create(struct inode *parent, struct dentry *dentry,
> +			     umode_t mode, int is_dir)
> +{
> +	struct vboxsf_inode *sf_parent_i = VBOXSF_I(parent);
> +	struct vboxsf_sbi *sbi = VBOXSF_SBI(parent->i_sb);
> +	struct shfl_createparms params = {};
> +	int err;
> +
> +	params.handle = SHFL_HANDLE_NIL;
> +	params.create_flags = SHFL_CF_ACT_CREATE_IF_NEW |
> +			      SHFL_CF_ACT_FAIL_IF_EXISTS |
> +			      SHFL_CF_ACCESS_READWRITE |
> +			      (is_dir ? SHFL_CF_DIRECTORY : 0);
> +	params.info.attr.mode = (mode & 0777) |
> +				(is_dir ? SHFL_TYPE_DIRECTORY : SHFL_TYPE_FILE);
> +	params.info.attr.additional = SHFLFSOBJATTRADD_NOTHING;
> +
> +	err = vboxsf_create_at_dentry(dentry, &params);

That's... interesting.  What should happen if you race with rename of
grandparent?  Note that *parent* is locked here; no deeper ancestors
are.

The same goes for removals.

> +static const char *vboxsf_get_link(struct dentry *dentry, struct inode *inode,
> +				   struct delayed_call *done)
> +{
> +	struct vboxsf_sbi *sbi = VBOXSF_SBI(inode->i_sb);
> +	struct shfl_string *path;
> +	char *link;
> +	int err;
> +
> +	if (!dentry)
> +		return ERR_PTR(-ECHILD);
> +
> +	path = vboxsf_path_from_dentry(sbi, dentry);
> +	if (IS_ERR(path))
> +		return (char *)path;

ERR_CAST(path)

> +	/** No additional information is available / requested. */
> +	SHFLFSOBJATTRADD_NOTHING = 1,

<unprintable>
Well, unpronounceable, actually...

> +	switch (opt) {
> +	case opt_nls:
> +		if (fc->purpose != FS_CONTEXT_FOR_MOUNT) {
> +			vbg_err("vboxsf: Cannot reconfigure nls option\n");
> +			return -EINVAL;
> +		}
> +		ctx->nls_name = param->string;
> +		param->string = NULL;

Umm...  What happens if you are given several such?  A leak?

> +{
> +	int err;
> +
> +	err = vboxsf_setup();
> +	if (err)
> +		return err;
> +
> +	return vfs_get_super(fc, vfs_get_independent_super, vboxsf_fill_super);

	return get_tree_nodev(fc, vboxsf_fill_super);
please,

> +static int vboxsf_reconfigure(struct fs_context *fc)
> +{
> +	struct vboxsf_sbi *sbi = VBOXSF_SBI(fc->root->d_sb);
> +	struct vboxsf_fs_context *ctx = fc->fs_private;
> +	struct inode *iroot;
> +
> +	iroot = ilookup(fc->root->d_sb, 0);
> +	if (!iroot)
> +		return -ENOENT;

Huh?  If that's supposed to be root directory inode, what's wrong
with ->d_sb->s_root->d_inode?

> +	path = dentry_path_raw(dentry, buf, PATH_MAX);
> +	if (IS_ERR(path)) {
> +		__putname(buf);
> +		return (struct shfl_string *)path;

ERR_CAST(path)...