linux-ext4 - Re: [PATCH] ext4: Clear lockdep subtype for quota files on quota off

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]

Date:   Fri, 5 May 2017 14:20:38 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Jan Kara <jack@...e.cz>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org,
        Ross Zwisler <ross.zwisler@...ux.intel.com>
Subject: Re: [PATCH] ext4: Clear lockdep subtype for quota files on quota off

On May 5, 2017, at 2:53 AM, Jan Kara <jack@...e.cz> wrote:
> 
> Quota files have special ranking of i_data_sem lock. We inform lockdep
> about it when turning on quotas however when turning quotas off, we
> don't clear the lockdep subclass from i_data_sem lock and thus when the
> inode gets later reused for a normal file or directory, lockdep gets
> confused and complains about possible deadlocks. Fix the problem by
> resetting lockdep subclass of i_data_sem on quota off.
> 
> Reported-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> Signed-off-by: Jan Kara <jack@...e.cz>

Fixes: daf647d2dd58cec59570d7698a45b98e580f2076
Reviewed-by: Andreas Dilger <adilger@...ger.ca>
---
The change to skip clearing the NOATIME/IMMUTABLE inode flags and times
on every mount/unmount when the journaled quota feature is enabled is
good, since it avoids needless overhead, but isn't really described in
the commit comment.  This isn't directly related to the lockdep, but
rather improving 957153fce8d2 "ext4: Set flags on quota files directly"
AFAICS.

It might be worthwhile to add a line to this commit like:

 Don't clear NOATIME/IMMUTABLE flags when journaled quota is enabled.

It also looks like ext4_quota_on() could use a similar check, to avoid
setting the NOATIME/IMMUTABLE flags on every mount if they are already
set.

I guess it isn't harmful to clear IMMUTABLE in the case of non-journaled
quotas, to maximize compatibility with older quota utilities, so long as
we don't incur this needless overhead for the newer journaled quota case.


There is a separate issue of what to do if the filesystem wasn't unmounted
cleanly and IMMUTABLE is still set?  If the userspace quotacheck always
clears IMMUTABLE when it is run, then there isn't much benefit in setting
the IMMUTABLE flag in the first place.  Is there some other way that the
userspace quota utilities know whether it is safe to update the quota files?

One possibility would be to use a similar open(O_EXCL) hack as we use
with block devices to avoid userspace trying to modify the quota file while
the kernel is using it?

Cheers, Andreas

> ---
> fs/ext4/super.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index a9c72e39a4ee..77043dc7f704 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -846,14 +846,9 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
> {
> 	int type;
> 
> -	if (ext4_has_feature_quota(sb)) {
> -		dquot_disable(sb, -1,
> -			      DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED);
> -	} else {
> -		/* Use our quota_off function to clear inode flags etc. */
> -		for (type = 0; type < EXT4_MAXQUOTAS; type++)
> -			ext4_quota_off(sb, type);
> -	}
> +	/* Use our quota_off function to clear inode flags etc. */
> +	for (type = 0; type < EXT4_MAXQUOTAS; type++)
> +		ext4_quota_off(sb, type);
> }
> #else
> static inline void ext4_quota_off_umount(struct super_block *sb)
> @@ -5476,7 +5471,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
> 		goto out;
> 
> 	err = dquot_quota_off(sb, type);
> -	if (err)
> +	if (err || ext4_has_feature_quota(sb))
> 		goto out_put;
> 
> 	inode_lock(inode);
> @@ -5496,6 +5491,7 @@ static int ext4_quota_off(struct super_block *sb, int type)
> out_unlock:
> 	inode_unlock(inode);
> out_put:
> +	lockdep_set_quota_inode(inode, I_DATA_SEM_NORMAL);
> 	iput(inode);
> 	return err;
> out:
> --
> 2.12.0
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)