On Thu 28-01-21 14:35:52, Christoph Hellwig wrote:

quoted

+	struct path path, *pathp = NULL;
+	struct path mountpath;
+	bool excl = false, thawed = false;
+	int ret;
+
+	cmds = cmd >> SUBCMDSHIFT;
+	type = cmd & SUBCMDMASK;

Personal pet peeve: it would be nice to just initialize cmds and
type on their declaration line, or while we're at it declutter
this a bit and remove the separate cmds variable:

	unsigned int type = cmd & SUBCMDMASK;

	cmd >>= SUBCMDSHIFT;

Yeah, whatever :)

quoted

+	/*
+	 * Path for quotaon has to be resolved before grabbing superblock
+	 * because that gets s_umount sem which is also possibly needed by path
+	 * resolution (think about autofs) and thus deadlocks could arise.
+	 */
+	if (cmds == Q_QUOTAON) {
+		ret = user_path_at(AT_FDCWD, addr,
+				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &path);
+		if (ret)
+			pathp = ERR_PTR(ret);
+		else
+			pathp = &path;
+	}
+
+	ret = user_path_at(AT_FDCWD, mountpoint,
+			     LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
+	if (ret)
+		goto out;

I don't think we need two path lookups here, we can path the same path
to the command for quotaon.

Hum, let me think out loud. The path we pass to Q_QUOTAON is a path to
quota file - unless the filesystem stores quota in hidden files in which
case this argument is just ignored. You're right we could require that
specifically for Q_QUOTAON, the mountpoint path would actually need to
point to the quota file if it is relevant, otherwise anywhere in the
appropriate filesystem. We don't allow quota file to reside on a different
filesystem (for a past decade or so) so it should work fine.

So the only problem I have is whether requiring the mountpoint argument to
point quota file for Q_QUOTAON isn't going to be somewhat confusing to
users. At the very least it would require some careful explanation in the
manpage to explain the difference between quotactl_path() and quotactl()
in this regard. But is saving the second path for Q_QUOTAON really worth the
bother?

quoted

+	if (quotactl_cmd_onoff(cmds)) {
+		excl = true;
+		thawed = true;
+	} else if (quotactl_cmd_write(cmds)) {
+		thawed = true;
+	}
+
+	if (thawed) {
+		ret = mnt_want_write(mountpath.mnt);
+		if (ret)
+			goto out1;
+	}
+
+	sb = mountpath.dentry->d_inode->i_sb;
+
+	if (excl)
+		down_write(&sb->s_umount);
+	else
+		down_read(&sb->s_umount);

Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
could probably simplify this down do:

	if (quotactl_cmd_write(cmd)) {

This needs to be (quotactl_cmd_write(cmd) || quotactl_cmd_onoff(cmd)).
Otherwise I agree what you suggest is somewhat more readable given how
small the function is.

		ret = mnt_want_write(path.mnt);
		if (ret)
			goto out1;
	}
	if (quotactl_cmd_onoff(cmd))
		down_write(&sb->s_umount);
	else
		down_read(&sb->s_umount);

and duplicate the checks after the do_quotactl call.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR