linux-kernel - Re: [PATCH 1/2] quota: Add mountpath based quota support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210128143552.GA2042235@infradead.org>
Date:   Thu, 28 Jan 2021 14:35:52 +0000
From:   Christoph Hellwig <hch@...radead.org>
To:     Sascha Hauer <s.hauer@...gutronix.de>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Christoph Hellwig <hch@...radead.org>, kernel@...gutronix.de,
        Jan Kara <jack@...e.com>, Richard Weinberger <richard@....at>
Subject: Re: [PATCH 1/2] quota: Add mountpath based quota support

> +	uint cmds, type;
> +	struct super_block *sb = NULL;

I don't think sb needs the NULL initialization.

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

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


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