linux-ext4 - Re: [PATCH] iversion: make inode_cmp_iversion{+raw} return bool instead of s64

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:   Tue, 30 Jan 2018 17:50:05 +0000
From:   Trond Myklebust <trondmy@...marydata.com>
To:     "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
        "jlayton@...nel.org" <jlayton@...nel.org>
CC:     "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "tytso@....edu" <tytso@....edu>
Subject: Re: [PATCH] iversion: make inode_cmp_iversion{+raw} return bool
 instead of s64

On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
> From: Jeff Layton <jlayton@...hat.com>
> 
> As Linus points out:
> 
>     The inode_cmp_iversion{+raw}() functions are pure and utter crap.
> 
>     Why?
> 
>     You say that they return 0/negative/positive, but they do so in a
>     completely broken manner. They return that ternary value as the
>     sequence number difference in a 's64', which means that if you
>     actually care about that ternary value, and do the *sane* thing
> that
>     the kernel-doc of the function implies is the right thing, you
> would
>     do
> 
>         int cmp = inode_cmp_iversion(inode, old);
>         if (cmp < 0 ...
> 
>     and as a result you get code that looks sane, but that doesn't
>     actually *WORK* right.
> 
> Since none of the callers actually care about the ternary value here,
> convert the inode_cmp_iversion{+raw} functions to just return a
> boolean
> value (false for matching, true for non-matching).
> 
> This matches the existing use of these functions just fine, and makes
> it
> simple to convert them to return a ternary value in the future if we
> grow callers that need it.
> 
> Signed-off-by: Jeff Layton <jlayton@...hat.com>
> ---
>  include/linux/iversion.h | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/iversion.h b/include/linux/iversion.h
> index 858463fca249..ace32775c5f0 100644
> --- a/include/linux/iversion.h
> +++ b/include/linux/iversion.h
> @@ -309,13 +309,13 @@ inode_query_iversion(struct inode *inode)
>   * @inode: inode to check
>   * @old: old value to check against its i_version
>   *
> - * Compare the current raw i_version counter with a previous one.
> Returns 0 if
> - * they are the same or non-zero if they are different.
> + * Compare the current raw i_version counter with a previous one.
> Returns false
> + * if they are the same or true if they are different.
>   */
> -static inline s64
> +static inline bool
>  inode_cmp_iversion_raw(const struct inode *inode, u64 old)
>  {
> -	return (s64)inode_peek_iversion_raw(inode) - (s64)old;
> +	return inode_peek_iversion_raw(inode) != old;
>  }
>  
>  /**
> @@ -323,19 +323,17 @@ inode_cmp_iversion_raw(const struct inode
> *inode, u64 old)
>   * @inode: inode to check
>   * @old: old value to check against its i_version
>   *
> - * Compare an i_version counter with a previous one. Returns 0 if
> they are
> - * the same, a positive value if the one in the inode appears newer
> than @old,
> - * and a negative value if @old appears to be newer than the one in
> the
> - * inode.
> + * Compare an i_version counter with a previous one. Returns false
> if they are
> + * the same, and true if they are different.
>   *
>   * Note that we don't need to set the QUERIED flag in this case, as
> the value
>   * in the inode is not being recorded for later use.
>   */
>  
> -static inline s64
> +static inline bool
>  inode_cmp_iversion(const struct inode *inode, u64 old)
>  {
> -	return (s64)(inode_peek_iversion_raw(inode) &
> ~I_VERSION_QUERIED) -
> -	       (s64)(old << I_VERSION_QUERIED_SHIFT);
> +	return (inode_peek_iversion_raw(inode) & ~I_VERSION_QUERIED)
> !=
> +			(old << I_VERSION_QUERIED_SHIFT);
>  }

Is there any reason why this couldn't just use inode_peek_iversion()
instead of having to both mask the output from
inode_peek_iversion_raw() and shift 'old'?

>  #endif
-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@...marydata.com