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  PHC 
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 20 Nov 2019 22:23:40 +1100
From:   Matthew Bobrowski <>
To:     Ritesh Harjani <>
Subject: Re: [RFCv3 2/4] ext4: Add ext4_ilock & ext4_iunlock API

On Wed, Nov 20, 2019 at 10:30:22AM +0530, Ritesh Harjani wrote:
> This adds ext4_ilock/iunlock types of APIs.
> This is the preparation APIs to make shared
> locking/unlocking & restarting with exclusive
> locking/unlocking easier in next patch.

*scratches head*

A nit, but what's with the changelog wrapping at like ~40 characters?

> +#define EXT4_IOLOCK_EXCL	(1 << 0)
> +#define EXT4_IOLOCK_SHARED	(1 << 1)
> +static inline void ext4_ilock(struct inode *inode, unsigned int iolock)
> +{
> +	if (iolock == EXT4_IOLOCK_EXCL)
> +		inode_lock(inode);
> +	else
> +		inode_lock_shared(inode);
> +}
> +
> +static inline void ext4_iunlock(struct inode *inode, unsigned int iolock)
> +{
> +	if (iolock == EXT4_IOLOCK_EXCL)
> +		inode_unlock(inode);
> +	else
> +		inode_unlock_shared(inode);
> +}
> +
> +static inline int ext4_ilock_nowait(struct inode *inode, unsigned int iolock)
> +{
> +	if (iolock == EXT4_IOLOCK_EXCL)
> +		return inode_trylock(inode);
> +	else
> +		return inode_trylock_shared(inode);
> +}

Is it really necessary for all these helpers to actually have the
'else' statement? Could we not just return/set whatever takes the
'else' branch directly from the end of these functions? I think it
would be cleaner that way.

/me doesn't really like the naming of these functions either.

What's people's opinion on changing these for example:
   - ext4_inode_lock()
   - ext4_inode_unlock()

Or, better yet, is there any reason why we've never actually
considered naming such functions to have the verb precede the actual
object that we're performing the operation on? In my opinion, it
totally makes way more sense from a code readability standpoint and
overall intent of the function. For example:
   - ext4_lock_inode()
   - ext4_unlock_inode()

> +static inline void ext4_ilock_demote(struct inode *inode, unsigned int iolock)
> +{
> +	BUG_ON(iolock != EXT4_IOLOCK_EXCL);
> +	downgrade_write(&inode->i_rwsem);
> +}
> +

Same principle would also apply here.

On an ending note, I'm not really sure that I like the name of these
macros. Like, for example, expand the macro 'EXT4_IOLOCK_EXCL' into
plain english words as if you were reading it. This would translate to
EXCLUSIVE'. Just flipping the words around make a significant
improvement for overall readability i.e. 'EXT4_EXCL_IOLOCK', which
would expand out to 'EXT4 EXCLUSIVE IO LOCK'. Speaking of, is there
any reason why we don't mention 'INODE' here seeing as though that's
the object we're actually protecting by taking one of these locking


Powered by blists - more mailing lists