linux-kernel - Re: [PATCH 1/4] lockd: convert nlm_host.h_count from atomic_t to refcount

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] [day] [month] [year] [list]

Date:   Wed, 24 Jan 2018 16:09:50 -0500
From:   "J. Bruce Fields" <bfields@...ldses.org>
To:     Trond Myklebust <trondmy@...il.com>
Cc:     "Reshetova, Elena" <elena.reshetova@...el.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jlayton@...nel.org" <jlayton@...nel.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>
Subject: Re: [PATCH 1/4] lockd: convert nlm_host.h_count from atomic_t to
 refcount_t

On Tue, Jan 23, 2018 at 07:47:31PM -0500, Trond Myklebust wrote:
> Sorry I forgot about the issues with the server garbage collector, and
> I applied these patches to my linux-next a couple of weeks ago.

Whoops, OK, so who's taking those patches anyway?

> What say we fix the issue with something like the following?
...
> @@ -662,8 +664,7 @@ nlm_gc_hosts(struct net *net)
>  	for_each_host_safe(host, next, chain, nlm_server_hosts) {
>  		if (net && host->net != net)
>  			continue;
> -		if (refcount_read(&host->h_count) || host->h_inuse
> -		 || time_before(jiffies, host->h_expires)) {
> +		if (host->h_inuse || time_before(jiffies, host->h_expires)) {

Can you really just drop the h_count check?

Oh, I see:

> @@ -671,7 +672,8 @@ nlm_gc_hosts(struct net *net)
>  				host->net->ns.inum);
>  			continue;
>  		}
> -		nlm_destroy_host_locked(host);
> +		if (refcount_dec_if_one(&host->h_count))
> +			nlm_destroy_host_locked(host);

So this is check that replaces it.

Makes sense to me, thanks.  ACK to the patch.

--b.

>  	}
>  
>  	if (net) {
> -- 
> 2.14.3
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@...marydata.com