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: <20210204073414.GA126863@infradead.org>
Date:   Thu, 4 Feb 2021 07:34:14 +0000
From:   Christoph Hellwig <hch@...radead.org>
To:     Jan Kara <jack@...e.cz>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de, Jan Kara <jack@...e.com>,
        Richard Weinberger <richard@....at>
Subject: Re: [PATCH 1/2] quota: Add mountpath based quota support

On Tue, Feb 02, 2021 at 07:02:41PM +0100, Jan Kara wrote:
> 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?

I find the doubled path argument a really horrible API, so I'd pretty
strongly prefer to avoid that.

> > 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.

The way I read quotactl_cmd_write, it only special cases a few commands
and returns 0 there, so we should not need the extra quotactl_cmd_onoff
call, as all those commands are not explicitly listed.