linux-kernel - Re: [PATCH v3 19/19] fs: handle inode->i

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: <20171218163418.GF30350@quack2.suse.cz>
Date:   Mon, 18 Dec 2017 17:34:18 +0100
From:   Jan Kara <jack@...e.cz>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk, linux-nfs@...r.kernel.org,
        bfields@...ldses.org, neilb@...e.de, jack@...e.de,
        linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, linux-xfs@...r.kernel.org,
        darrick.wong@...cle.com, david@...morbit.com,
        linux-btrfs@...r.kernel.org, clm@...com, jbacik@...com,
        dsterba@...e.com, linux-integrity@...r.kernel.org,
        zohar@...ux.vnet.ibm.com, dmitry.kasatkin@...il.com,
        linux-afs@...ts.infradead.org, dhowells@...hat.com,
        jaltman@...istor.com
Subject: Re: [PATCH v3 19/19] fs: handle inode->i_version more efficiently

On Mon 18-12-17 10:11:56, Jeff Layton wrote:
>  static inline bool
>  inode_maybe_inc_iversion(struct inode *inode, bool force)
>  {
> -	atomic64_t *ivp = (atomic64_t *)&inode->i_version;
> +	u64 cur, old, new;
>  
> -	atomic64_inc(ivp);
> +	cur = (u64)atomic64_read(&inode->i_version);
> +	for (;;) {
> +		/* If flag is clear then we needn't do anything */
> +		if (!force && !(cur & I_VERSION_QUERIED))
> +			return false;

The fast path here misses any memory barrier. Thus it seems this query
could be in theory reordered before any store that happened to modify the
inode? Or maybe we could race and miss the fact that in fact this i_version
has already been queried? But maybe there's some higher level locking that
makes sure this is all a non-issue... But in that case it would deserve
some comment I guess.

> +
> +		/* Since lowest bit is flag, add 2 to avoid it */
> +		new = (cur & ~I_VERSION_QUERIED) + I_VERSION_INCREMENT;
> +
> +		old = atomic64_cmpxchg(&inode->i_version, cur, new);
> +		if (likely(old == cur))
> +			break;
> +		cur = old;
> +	}
>  	return true;
>  }
>  

...

>  static inline u64
>  inode_query_iversion(struct inode *inode)
>  {
> -	return inode_peek_iversion(inode);
> +	u64 cur, old, new;
> +
> +	cur = atomic64_read(&inode->i_version);
> +	for (;;) {
> +		/* If flag is already set, then no need to swap */
> +		if (cur & I_VERSION_QUERIED)
> +			break;
> +
> +		new = cur | I_VERSION_QUERIED;
> +		old = atomic64_cmpxchg(&inode->i_version, cur, new);
> +		if (old == cur)
> +			break;
> +		cur = old;
> +	}

Why not just use atomic64_or() here?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR