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  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:   Sat, 19 Dec 2020 11:23:47 -0500
From:   Tejun Heo <tj@...nel.org>
To:     Ian Kent <raven@...maw.net>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Fox Chen <foxhlchen@...il.com>, akpm@...ux-foundation.org,
        dhowells@...hat.com, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, miklos@...redi.hu,
        ricklind@...ux.vnet.ibm.com, sfr@...b.auug.org.au,
        viro@...iv.linux.org.uk
Subject: Re: [PATCH v2 0/6] kernfs: proposed locking and concurrency
 improvement

Hello,

On Sat, Dec 19, 2020 at 03:08:13PM +0800, Ian Kent wrote:
> And looking further I see there's a race that kernfs can't do anything
> about between kernfs_refresh_inode() and fs/inode.c:update_times().

Do kernfs files end up calling into that path tho? Doesn't look like it to
me but if so yeah we'd need to override the update_time for kernfs.

> +static int kernfs_iop_update_time(struct inode *inode, struct timespec64 *time, int flags)
>  {
> -	struct inode *inode = d_inode(path->dentry);
>  	struct kernfs_node *kn = inode->i_private;
> +	struct kernfs_iattrs *attrs;
>  
>  	mutex_lock(&kernfs_mutex);
> +	attrs = kernfs_iattrs(kn);
> +	if (!attrs) {
> +		mutex_unlock(&kernfs_mutex);
> +		return -ENOMEM;
> +	}
> +
> +	/* Which kernfs node attributes should be updated from
> +	 * time?
> +	 */
> +
>  	kernfs_refresh_inode(kn, inode);
>  	mutex_unlock(&kernfs_mutex);

I don't see how this would reflect the changes from kernfs_setattr() into
the attached inode. This would actually make the attr updates obviously racy
- the userland visible attrs would be stale until the inode gets reclaimed
and then when it gets reinstantiated it'd show the latest information.

That said, if you wanna take the direction where attr updates are reflected
to the associated inode when the change occurs, which makes sense, the right
thing to do would be making kernfs_setattr() update the associated inode if
existent.

Thanks.

-- 
tejun

Powered by blists - more mailing lists