linux-kernel - Re: [PATCH RFC v2 4/4] 9p: fix race issue in fid contention.

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:   Wed, 23 Sep 2020 16:49:53 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Jianyong Wu <jianyong.wu@....com>
Cc:     ericvh@...il.com, lucho@...kov.net, qemu_oss@...debyte.com,
        groug@...d.org, v9fs-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, justin.he@....com
Subject: Re: [PATCH RFC v2 4/4] 9p: fix race issue in fid contention.

Overall looks good; a few comments.

Jianyong Wu wrote on Wed, Sep 23, 2020:
> open-unlink-f*syscall test:
> I have tested for f*syscall include: ftruncate fstat fchown fchmod faccessat.

Given the other thread, what did you test this with?
Since qemu doesn't work apparently do you have a in-house server at arm
I could test?
(I'll try with ganesha otherwise, it keeps files open so it should work
I think...)

> +	atomic_set(&fid->count, 1);

I kind of like the refcount API becauese it has some extra overflow
checks; but it requires a bit more work around clunk (instead of bailing
out early if counter hits 0, you need to have it call a separate
function in case it does)

That's mostly esthetics though I'm not going to fuss over that.

> @@ -74,6 +77,7 @@ static struct p9_fid *v9fs_fid_find_inode(struct inode *inode, kuid_t uid)
>  void v9fs_open_fid_add(struct inode *inode, struct p9_fid *fid)
>  {
>  	spin_lock(&inode->i_lock);
> +	atomic_set(&fid->count, 1);

Hm, that should be done at fid creation time in net/9p/client.c
p9_fid_create ; no ?
(you do it there already, I don't see what reseting count here brings
except confusion)

> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index dfa11df02818..1fed96546728 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -22,6 +22,14 @@ static inline struct p9_fid *clone_fid(struct p9_fid *fid)
>  }
>  static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
>  {
> -	return clone_fid(v9fs_fid_lookup(dentry));
> +	struct p9_fid *fid, *nfid;
> +
> +	fid = v9fs_fid_lookup(dentry);
> +	if (!fid || IS_ERR(fid))
> +		return fid;
> +
> +	nfid = p9_client_walk(fid, 0, NULL, 1);

I think you clone_fid() here is slightly easier to understand; everyone
doesn't know that a walk with no component is a clone.
The compiler will optimize that IS_ERR(fid) is checked twice, it's fine.

> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index ce7882da8e86..58ed9bd306bd 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -140,10 +140,16 @@ struct p9_client {
>   *
>   * TODO: This needs lots of explanation.
>   */
> +enum fid_source {
> +	FID_FROM_OTHER,
> +	FID_FROM_INODE,
> +	FID_FROM_DENTRY,
> +};

leftovers from previous iteration.

Overall looks good to me.
I'd need to spend some time checking the actual counting part &
hammering the fs a bit then confirming no fid got forgotten (there's a
pr_info at umount time) but I'm happy with this ; thanks!

-- 
Dominique