This series, for lockd component, replaces atomic_t reference
counters with the new refcount_t type and API (see include/linux/refcount.h).
By doing this we prevent intentional or accidental
underflows or overflows that can led to use-after-free vulnerabilities.
The patches are fully independent and can be cherry-picked separately.
If there are no objections to the patches, please merge them via respective tree.
Elena Reshetova (4):
lockd: convert nlm_host.h_count from atomic_t to refcount_t
lockd: convert nsm_handle.sm_count from atomic_t to refcount_t
lockd: convert nlm_lockowner.count from atomic_t to refcount_t
lockd: convert nlm_rqst.a_count from atomic_t to refcount_t
fs/lockd/clntproc.c | 14 +++++++-------
fs/lockd/host.c | 16 ++++++++--------
fs/lockd/mon.c | 14 +++++++-------
fs/lockd/svcproc.c | 2 +-
include/linux/lockd/lockd.h | 9 +++++----
5 files changed, 28 insertions(+), 27 deletions(-)
--
2.7.4
atomic_t variables are currently used to implement reference
counters with the following properties:
- counter is initialized to 1 using atomic_set()
- a resource is freed upon counter reaching zero
- once counter reaches zero, its further
increments aren't allowed
- counter schema uses basic atomic operations
(set, inc, inc_not_zero, dec_and_test, etc.)
Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.
The variable nlm_rqst.a_count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.
**Important note for maintainers:
Some functions from refcount_t API defined in lib/refcount.c
have different memory ordering guarantees than their atomic
counterparts.
The full comparison can be seen in
https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
in state to be merged to the documentation tree.
Normally the differences should not matter since refcount_t provides
enough guarantees to satisfy the refcounting use cases, but in
some rare cases it might matter.
Please double check that you don't have some undocumented
memory guarantees for this variable usage.
For the nlm_rqst.a_count it might make a difference
in following places:
- nlmclnt_release_call() and nlmsvc_release_call(): decrement
in refcount_dec_and_test() only
provides RELEASE ordering and control dependency on success
vs. fully ordered atomic counterpart
Suggested-by: Kees Cook <[email protected]>
Reviewed-by: David Windsor <[email protected]>
Reviewed-by: Hans Liljestrand <[email protected]>
Signed-off-by: Elena Reshetova <[email protected]>
---
fs/lockd/clntproc.c | 8 ++++----
fs/lockd/svcproc.c | 2 +-
include/linux/lockd/lockd.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 112173d..a2c0dfc 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -204,7 +204,7 @@ struct nlm_rqst *nlm_alloc_call(struct nlm_host *host)
for(;;) {
call = kzalloc(sizeof(*call), GFP_KERNEL);
if (call != NULL) {
- atomic_set(&call->a_count, 1);
+ refcount_set(&call->a_count, 1);
locks_init_lock(&call->a_args.lock.fl);
locks_init_lock(&call->a_res.lock.fl);
call->a_host = nlm_get_host(host);
@@ -222,7 +222,7 @@ void nlmclnt_release_call(struct nlm_rqst *call)
{
const struct nlmclnt_operations *nlmclnt_ops = call->a_host->h_nlmclnt_ops;
- if (!atomic_dec_and_test(&call->a_count))
+ if (!refcount_dec_and_test(&call->a_count))
return;
if (nlmclnt_ops && nlmclnt_ops->nlmclnt_release_call)
nlmclnt_ops->nlmclnt_release_call(call->a_callback_data);
@@ -678,7 +678,7 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
goto out;
}
- atomic_inc(&req->a_count);
+ refcount_inc(&req->a_count);
status = nlmclnt_async_call(nfs_file_cred(fl->fl_file), req,
NLMPROC_UNLOCK, &nlmclnt_unlock_ops);
if (status < 0)
@@ -769,7 +769,7 @@ static int nlmclnt_cancel(struct nlm_host *host, int block, struct file_lock *fl
nlmclnt_setlockargs(req, fl);
req->a_args.block = block;
- atomic_inc(&req->a_count);
+ refcount_inc(&req->a_count);
status = nlmclnt_async_call(nfs_file_cred(fl->fl_file), req,
NLMPROC_CANCEL, &nlmclnt_cancel_ops);
if (status == 0 && req->a_res.status == nlm_lck_denied)
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index 0d670c5..ea77c66 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -295,7 +295,7 @@ static void nlmsvc_callback_exit(struct rpc_task *task, void *data)
void nlmsvc_release_call(struct nlm_rqst *call)
{
- if (!atomic_dec_and_test(&call->a_count))
+ if (!refcount_dec_and_test(&call->a_count))
return;
nlmsvc_release_host(call->a_host);
kfree(call);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 86d012a..4fd95db 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -137,7 +137,7 @@ struct nlm_wait;
*/
#define NLMCLNT_OHSIZE ((__NEW_UTS_LEN) + 10u)
struct nlm_rqst {
- atomic_t a_count;
+ refcount_t a_count;
unsigned int a_flags; /* initial RPC task flags */
struct nlm_host * a_host; /* host handle */
struct nlm_args a_args; /* arguments */
--
2.7.4
atomic_t variables are currently used to implement reference
counters with the following properties:
- counter is initialized to 1 using atomic_set()
- a resource is freed upon counter reaching zero
- once counter reaches zero, its further
increments aren't allowed
- counter schema uses basic atomic operations
(set, inc, inc_not_zero, dec_and_test, etc.)
Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.
The variable nlm_lockowner.count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.
**Important note for maintainers:
Some functions from refcount_t API defined in lib/refcount.c
have different memory ordering guarantees than their atomic
counterparts.
The full comparison can be seen in
https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
in state to be merged to the documentation tree.
Normally the differences should not matter since refcount_t provides
enough guarantees to satisfy the refcounting use cases, but in
some rare cases it might matter.
Please double check that you don't have some undocumented
memory guarantees for this variable usage.
For the nlm_lockowner.count it might make a difference
in following places:
- nlm_put_lockowner(): decrement in refcount_dec_and_lock() only
provides RELEASE ordering, control dependency on success and
holds a spin lock on success vs. fully ordered atomic counterpart.
No changes in spin lock guarantees.
Suggested-by: Kees Cook <[email protected]>
Reviewed-by: David Windsor <[email protected]>
Reviewed-by: Hans Liljestrand <[email protected]>
Signed-off-by: Elena Reshetova <[email protected]>
---
fs/lockd/clntproc.c | 6 +++---
include/linux/lockd/lockd.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 066ac31..112173d 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -48,13 +48,13 @@ void nlmclnt_next_cookie(struct nlm_cookie *c)
static struct nlm_lockowner *nlm_get_lockowner(struct nlm_lockowner *lockowner)
{
- atomic_inc(&lockowner->count);
+ refcount_inc(&lockowner->count);
return lockowner;
}
static void nlm_put_lockowner(struct nlm_lockowner *lockowner)
{
- if (!atomic_dec_and_lock(&lockowner->count, &lockowner->host->h_lock))
+ if (!refcount_dec_and_lock(&lockowner->count, &lockowner->host->h_lock))
return;
list_del(&lockowner->list);
spin_unlock(&lockowner->host->h_lock);
@@ -105,7 +105,7 @@ static struct nlm_lockowner *nlm_find_lockowner(struct nlm_host *host, fl_owner_
res = __nlm_find_lockowner(host, owner);
if (res == NULL && new != NULL) {
res = new;
- atomic_set(&new->count, 1);
+ refcount_set(&new->count, 1);
new->owner = owner;
new->pid = __nlm_alloc_pid(host);
new->host = nlm_get_host(host);
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index cded0ad..86d012a 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -123,7 +123,7 @@ static inline struct sockaddr *nlm_srcaddr(const struct nlm_host *host)
*/
struct nlm_lockowner {
struct list_head list;
- atomic_t count;
+ refcount_t count;
struct nlm_host *host;
fl_owner_t owner;
--
2.7.4
atomic_t variables are currently used to implement reference
counters with the following properties:
- counter is initialized to 1 using atomic_set()
- a resource is freed upon counter reaching zero
- once counter reaches zero, its further
increments aren't allowed
- counter schema uses basic atomic operations
(set, inc, inc_not_zero, dec_and_test, etc.)
Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.
The variable nlm_host.h_count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.
**Important note for maintainers:
Some functions from refcount_t API defined in lib/refcount.c
have different memory ordering guarantees than their atomic
counterparts.
The full comparison can be seen in
https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
in state to be merged to the documentation tree.
Normally the differences should not matter since refcount_t provides
enough guarantees to satisfy the refcounting use cases, but in
some rare cases it might matter.
Please double check that you don't have some undocumented
memory guarantees for this variable usage.
For the nlm_host.h_count it might make a difference
in following places:
- nlmsvc_release_host(): decrement in refcount_dec()
provides RELEASE ordering, while original atomic_dec()
was fully unordered. Since the change is for better, it
should not matter.
- nlmclnt_release_host(): decrement in refcount_dec_and_test() only
provides RELEASE ordering and control dependency on success
vs. fully ordered atomic counterpart. It doesn't seem to
matter in this case since object freeing happens under mutex
lock anyway.
Suggested-by: Kees Cook <[email protected]>
Reviewed-by: David Windsor <[email protected]>
Reviewed-by: Hans Liljestrand <[email protected]>
Signed-off-by: Elena Reshetova <[email protected]>
---
fs/lockd/host.c | 14 +++++++-------
include/linux/lockd/lockd.h | 3 ++-
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 826a891..11b6832 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -151,7 +151,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
host->h_state = 0;
host->h_nsmstate = 0;
host->h_pidcount = 0;
- atomic_set(&host->h_count, 1);
+ refcount_set(&host->h_count, 1);
mutex_init(&host->h_mutex);
host->h_nextrebind = now + NLM_HOST_REBIND;
host->h_expires = now + NLM_HOST_EXPIRE;
@@ -290,7 +290,7 @@ void nlmclnt_release_host(struct nlm_host *host)
WARN_ON_ONCE(host->h_server);
- if (atomic_dec_and_test(&host->h_count)) {
+ if (refcount_dec_and_test(&host->h_count)) {
WARN_ON_ONCE(!list_empty(&host->h_lockowners));
WARN_ON_ONCE(!list_empty(&host->h_granted));
WARN_ON_ONCE(!list_empty(&host->h_reclaim));
@@ -410,7 +410,7 @@ void nlmsvc_release_host(struct nlm_host *host)
dprintk("lockd: release server host %s\n", host->h_name);
WARN_ON_ONCE(!host->h_server);
- atomic_dec(&host->h_count);
+ refcount_dec(&host->h_count);
}
/*
@@ -504,7 +504,7 @@ struct nlm_host * nlm_get_host(struct nlm_host *host)
{
if (host) {
dprintk("lockd: get host %s\n", host->h_name);
- atomic_inc(&host->h_count);
+ refcount_inc(&host->h_count);
host->h_expires = jiffies + NLM_HOST_EXPIRE;
}
return host;
@@ -593,7 +593,7 @@ static void nlm_complain_hosts(struct net *net)
if (net && host->net != net)
continue;
dprintk(" %s (cnt %d use %d exp %ld net %x)\n",
- host->h_name, atomic_read(&host->h_count),
+ host->h_name, refcount_read(&host->h_count),
host->h_inuse, host->h_expires, host->net->ns.inum);
}
}
@@ -662,11 +662,11 @@ nlm_gc_hosts(struct net *net)
for_each_host_safe(host, next, chain, nlm_server_hosts) {
if (net && host->net != net)
continue;
- if (atomic_read(&host->h_count) || host->h_inuse
+ if (refcount_read(&host->h_count) || host->h_inuse
|| time_before(jiffies, host->h_expires)) {
dprintk("nlm_gc_hosts skipping %s "
"(cnt %d use %d exp %ld net %x)\n",
- host->h_name, atomic_read(&host->h_count),
+ host->h_name, refcount_read(&host->h_count),
host->h_inuse, host->h_expires,
host->net->ns.inum);
continue;
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index d7d313f..39dfeea 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -17,6 +17,7 @@
#include <net/ipv6.h>
#include <linux/fs.h>
#include <linux/kref.h>
+#include <linux/refcount.h>
#include <linux/utsname.h>
#include <linux/lockd/bind.h>
#include <linux/lockd/xdr.h>
@@ -58,7 +59,7 @@ struct nlm_host {
u32 h_state; /* pseudo-state counter */
u32 h_nsmstate; /* true remote NSM state */
u32 h_pidcount; /* Pseudopids */
- atomic_t h_count; /* reference count */
+ refcount_t h_count; /* reference count */
struct mutex h_mutex; /* mutex for pmap binding */
unsigned long h_nextrebind; /* next portmap call */
unsigned long h_expires; /* eligible for GC */
--
2.7.4
atomic_t variables are currently used to implement reference
counters with the following properties:
- counter is initialized to 1 using atomic_set()
- a resource is freed upon counter reaching zero
- once counter reaches zero, its further
increments aren't allowed
- counter schema uses basic atomic operations
(set, inc, inc_not_zero, dec_and_test, etc.)
Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.
The variable nsm_handle.sm_count is used as pure reference counter.
Convert it to refcount_t and fix up the operations.
**Important note for maintainers:
Some functions from refcount_t API defined in lib/refcount.c
have different memory ordering guarantees than their atomic
counterparts.
The full comparison can be seen in
https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
in state to be merged to the documentation tree.
Normally the differences should not matter since refcount_t provides
enough guarantees to satisfy the refcounting use cases, but in
some rare cases it might matter.
Please double check that you don't have some undocumented
memory guarantees for this variable usage.
For the nsm_handle.sm_count it might make a difference
in following places:
- nsm_release(): decrement in refcount_dec_and_lock() only
provides RELEASE ordering, control dependency on success
and holds a spin lock on success vs. fully ordered atomic
counterpart. No change for the spin lock guarantees.
Suggested-by: Kees Cook <[email protected]>
Reviewed-by: David Windsor <[email protected]>
Reviewed-by: Hans Liljestrand <[email protected]>
Signed-off-by: Elena Reshetova <[email protected]>
---
fs/lockd/host.c | 2 +-
fs/lockd/mon.c | 14 +++++++-------
include/linux/lockd/lockd.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 11b6832..7d6ab72 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -114,7 +114,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
unsigned long now = jiffies;
if (nsm != NULL)
- atomic_inc(&nsm->sm_count);
+ refcount_inc(&nsm->sm_count);
else {
host = NULL;
nsm = nsm_get_handle(ni->net, ni->sap, ni->salen,
diff --git a/fs/lockd/mon.c b/fs/lockd/mon.c
index 96cfb29..654594e 100644
--- a/fs/lockd/mon.c
+++ b/fs/lockd/mon.c
@@ -191,7 +191,7 @@ void nsm_unmonitor(const struct nlm_host *host)
struct nsm_res res;
int status;
- if (atomic_read(&nsm->sm_count) == 1
+ if (refcount_read(&nsm->sm_count) == 1
&& nsm->sm_monitored && !nsm->sm_sticky) {
dprintk("lockd: nsm_unmonitor(%s)\n", nsm->sm_name);
@@ -279,7 +279,7 @@ static struct nsm_handle *nsm_create_handle(const struct sockaddr *sap,
if (unlikely(new == NULL))
return NULL;
- atomic_set(&new->sm_count, 1);
+ refcount_set(&new->sm_count, 1);
new->sm_name = (char *)(new + 1);
memcpy(nsm_addr(new), sap, salen);
new->sm_addrlen = salen;
@@ -337,13 +337,13 @@ struct nsm_handle *nsm_get_handle(const struct net *net,
cached = nsm_lookup_addr(&ln->nsm_handles, sap);
if (cached != NULL) {
- atomic_inc(&cached->sm_count);
+ refcount_inc(&cached->sm_count);
spin_unlock(&nsm_lock);
kfree(new);
dprintk("lockd: found nsm_handle for %s (%s), "
"cnt %d\n", cached->sm_name,
cached->sm_addrbuf,
- atomic_read(&cached->sm_count));
+ refcount_read(&cached->sm_count));
return cached;
}
@@ -388,12 +388,12 @@ struct nsm_handle *nsm_reboot_lookup(const struct net *net,
return cached;
}
- atomic_inc(&cached->sm_count);
+ refcount_inc(&cached->sm_count);
spin_unlock(&nsm_lock);
dprintk("lockd: host %s (%s) rebooted, cnt %d\n",
cached->sm_name, cached->sm_addrbuf,
- atomic_read(&cached->sm_count));
+ refcount_read(&cached->sm_count));
return cached;
}
@@ -404,7 +404,7 @@ struct nsm_handle *nsm_reboot_lookup(const struct net *net,
*/
void nsm_release(struct nsm_handle *nsm)
{
- if (atomic_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
+ if (refcount_dec_and_lock(&nsm->sm_count, &nsm_lock)) {
list_del(&nsm->sm_link);
spin_unlock(&nsm_lock);
dprintk("lockd: destroyed nsm_handle for %s (%s)\n",
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 39dfeea..cded0ad 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -84,7 +84,7 @@ struct nlm_host {
struct nsm_handle {
struct list_head sm_link;
- atomic_t sm_count;
+ refcount_t sm_count;
char *sm_mon_name;
char *sm_name;
struct sockaddr_storage sm_addr;
--
2.7.4
Thanks, applying all four for 4.16.--b.
On Wed, Nov 29, 2017 at 01:15:42PM +0200, Elena Reshetova wrote:
> This series, for lockd component, replaces atomic_t reference
> counters with the new refcount_t type and API (see include/linux/refcount.h).
> By doing this we prevent intentional or accidental
> underflows or overflows that can led to use-after-free vulnerabilities.
>
> The patches are fully independent and can be cherry-picked separately.
> If there are no objections to the patches, please merge them via respective tree.
>
> Elena Reshetova (4):
> lockd: convert nlm_host.h_count from atomic_t to refcount_t
> lockd: convert nsm_handle.sm_count from atomic_t to refcount_t
> lockd: convert nlm_lockowner.count from atomic_t to refcount_t
> lockd: convert nlm_rqst.a_count from atomic_t to refcount_t
>
> fs/lockd/clntproc.c | 14 +++++++-------
> fs/lockd/host.c | 16 ++++++++--------
> fs/lockd/mon.c | 14 +++++++-------
> fs/lockd/svcproc.c | 2 +-
> include/linux/lockd/lockd.h | 9 +++++----
> 5 files changed, 28 insertions(+), 27 deletions(-)
>
> --
> 2.7.4
> Thanks, applying all four for 4.16.--b.
Thank you very much!
Best Regards,
Elena.
>
> On Wed, Nov 29, 2017 at 01:15:42PM +0200, Elena Reshetova wrote:
> > This series, for lockd component, replaces atomic_t reference
> > counters with the new refcount_t type and API (see include/linux/refcount.h).
> > By doing this we prevent intentional or accidental
> > underflows or overflows that can led to use-after-free vulnerabilities.
> >
> > The patches are fully independent and can be cherry-picked separately.
> > If there are no objections to the patches, please merge them via respective
> tree.
> >
> > Elena Reshetova (4):
> > lockd: convert nlm_host.h_count from atomic_t to refcount_t
> > lockd: convert nsm_handle.sm_count from atomic_t to refcount_t
> > lockd: convert nlm_lockowner.count from atomic_t to refcount_t
> > lockd: convert nlm_rqst.a_count from atomic_t to refcount_t
> >
> > fs/lockd/clntproc.c | 14 +++++++-------
> > fs/lockd/host.c | 16 ++++++++--------
> > fs/lockd/mon.c | 14 +++++++-------
> > fs/lockd/svcproc.c | 2 +-
> > include/linux/lockd/lockd.h | 9 +++++----
> > 5 files changed, 28 insertions(+), 27 deletions(-)
> >
> > --
> > 2.7.4
On Wed, Nov 29, 2017 at 01:15:43PM +0200, Elena Reshetova wrote:
> atomic_t variables are currently used to implement reference
> counters with the following properties:
> - counter is initialized to 1 using atomic_set()
> - a resource is freed upon counter reaching zero
> - once counter reaches zero, its further
> increments aren't allowed
> - counter schema uses basic atomic operations
> (set, inc, inc_not_zero, dec_and_test, etc.)
Whoops, I forgot that this doesn't apply to h_count.
Well, it's confusing, because h_count is actually used in two different
ways: depending on whether a nlm_host represents a client or server, it
may have the above properties or not.
Inclined to drop this patch for now.
--b.
>
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
>
> The variable nlm_host.h_count is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
>
> **Important note for maintainers:
>
> Some functions from refcount_t API defined in lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.
> The full comparison can be seen in
> https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> in state to be merged to the documentation tree.
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in
> some rare cases it might matter.
> Please double check that you don't have some undocumented
> memory guarantees for this variable usage.
>
> For the nlm_host.h_count it might make a difference
> in following places:
> - nlmsvc_release_host(): decrement in refcount_dec()
> provides RELEASE ordering, while original atomic_dec()
> was fully unordered. Since the change is for better, it
> should not matter.
> - nlmclnt_release_host(): decrement in refcount_dec_and_test() only
> provides RELEASE ordering and control dependency on success
> vs. fully ordered atomic counterpart. It doesn't seem to
> matter in this case since object freeing happens under mutex
> lock anyway.
>
> Suggested-by: Kees Cook <[email protected]>
> Reviewed-by: David Windsor <[email protected]>
> Reviewed-by: Hans Liljestrand <[email protected]>
> Signed-off-by: Elena Reshetova <[email protected]>
> ---
> fs/lockd/host.c | 14 +++++++-------
> include/linux/lockd/lockd.h | 3 ++-
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 826a891..11b6832 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -151,7 +151,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
> host->h_state = 0;
> host->h_nsmstate = 0;
> host->h_pidcount = 0;
> - atomic_set(&host->h_count, 1);
> + refcount_set(&host->h_count, 1);
> mutex_init(&host->h_mutex);
> host->h_nextrebind = now + NLM_HOST_REBIND;
> host->h_expires = now + NLM_HOST_EXPIRE;
> @@ -290,7 +290,7 @@ void nlmclnt_release_host(struct nlm_host *host)
>
> WARN_ON_ONCE(host->h_server);
>
> - if (atomic_dec_and_test(&host->h_count)) {
> + if (refcount_dec_and_test(&host->h_count)) {
> WARN_ON_ONCE(!list_empty(&host->h_lockowners));
> WARN_ON_ONCE(!list_empty(&host->h_granted));
> WARN_ON_ONCE(!list_empty(&host->h_reclaim));
> @@ -410,7 +410,7 @@ void nlmsvc_release_host(struct nlm_host *host)
> dprintk("lockd: release server host %s\n", host->h_name);
>
> WARN_ON_ONCE(!host->h_server);
> - atomic_dec(&host->h_count);
> + refcount_dec(&host->h_count);
> }
>
> /*
> @@ -504,7 +504,7 @@ struct nlm_host * nlm_get_host(struct nlm_host *host)
> {
> if (host) {
> dprintk("lockd: get host %s\n", host->h_name);
> - atomic_inc(&host->h_count);
> + refcount_inc(&host->h_count);
> host->h_expires = jiffies + NLM_HOST_EXPIRE;
> }
> return host;
> @@ -593,7 +593,7 @@ static void nlm_complain_hosts(struct net *net)
> if (net && host->net != net)
> continue;
> dprintk(" %s (cnt %d use %d exp %ld net %x)\n",
> - host->h_name, atomic_read(&host->h_count),
> + host->h_name, refcount_read(&host->h_count),
> host->h_inuse, host->h_expires, host->net->ns.inum);
> }
> }
> @@ -662,11 +662,11 @@ nlm_gc_hosts(struct net *net)
> for_each_host_safe(host, next, chain, nlm_server_hosts) {
> if (net && host->net != net)
> continue;
> - if (atomic_read(&host->h_count) || host->h_inuse
> + if (refcount_read(&host->h_count) || host->h_inuse
> || time_before(jiffies, host->h_expires)) {
> dprintk("nlm_gc_hosts skipping %s "
> "(cnt %d use %d exp %ld net %x)\n",
> - host->h_name, atomic_read(&host->h_count),
> + host->h_name, refcount_read(&host->h_count),
> host->h_inuse, host->h_expires,
> host->net->ns.inum);
> continue;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index d7d313f..39dfeea 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -17,6 +17,7 @@
> #include <net/ipv6.h>
> #include <linux/fs.h>
> #include <linux/kref.h>
> +#include <linux/refcount.h>
> #include <linux/utsname.h>
> #include <linux/lockd/bind.h>
> #include <linux/lockd/xdr.h>
> @@ -58,7 +59,7 @@ struct nlm_host {
> u32 h_state; /* pseudo-state counter */
> u32 h_nsmstate; /* true remote NSM state */
> u32 h_pidcount; /* Pseudopids */
> - atomic_t h_count; /* reference count */
> + refcount_t h_count; /* reference count */
> struct mutex h_mutex; /* mutex for pmap binding */
> unsigned long h_nextrebind; /* next portmap call */
> unsigned long h_expires; /* eligible for GC */
> --
> 2.7.4
On Wed, Nov 29, 2017 at 01:15:43PM +0200, Elena Reshetova wrote:
> atomic_t variables are currently used to implement reference
> counters with the following properties:
> - counter is initialized to 1 using atomic_set()
> - a resource is freed upon counter reaching zero
> - once counter reaches zero, its further
> increments aren't allowed
> - counter schema uses basic atomic operations
> (set, inc, inc_not_zero, dec_and_test, etc.)
>Whoops, I forgot that this doesn't apply to h_count.
>Well, it's confusing, because h_count is actually used in two different
>ways: depending on whether a nlm_host represents a client or server, it
>may have the above properties or not.
So, what happens when it is not having the above properties? Is the object
being reused or?
I am just trying to understand if there is a way to fix this patch to work for the case
or is the drop is the only correct way to go.
Best Regards,
Elena.
>Inclined to drop this patch for now.
--b.
>
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
>
> The variable nlm_host.h_count is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
>
> **Important note for maintainers:
>
> Some functions from refcount_t API defined in lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.
> The full comparison can be seen in
> https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> in state to be merged to the documentation tree.
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in
> some rare cases it might matter.
> Please double check that you don't have some undocumented
> memory guarantees for this variable usage.
>
> For the nlm_host.h_count it might make a difference
> in following places:
> - nlmsvc_release_host(): decrement in refcount_dec()
> provides RELEASE ordering, while original atomic_dec()
> was fully unordered. Since the change is for better, it
> should not matter.
> - nlmclnt_release_host(): decrement in refcount_dec_and_test() only
> provides RELEASE ordering and control dependency on success
> vs. fully ordered atomic counterpart. It doesn't seem to
> matter in this case since object freeing happens under mutex
> lock anyway.
>
> Suggested-by: Kees Cook <[email protected]>
> Reviewed-by: David Windsor <[email protected]>
> Reviewed-by: Hans Liljestrand <[email protected]>
> Signed-off-by: Elena Reshetova <[email protected]>
> ---
> fs/lockd/host.c | 14 +++++++-------
> include/linux/lockd/lockd.h | 3 ++-
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 826a891..11b6832 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -151,7 +151,7 @@ static struct nlm_host *nlm_alloc_host(struct nlm_lookup_host_info *ni,
> host->h_state = 0;
> host->h_nsmstate = 0;
> host->h_pidcount = 0;
> - atomic_set(&host->h_count, 1);
> + refcount_set(&host->h_count, 1);
> mutex_init(&host->h_mutex);
> host->h_nextrebind = now + NLM_HOST_REBIND;
> host->h_expires = now + NLM_HOST_EXPIRE;
> @@ -290,7 +290,7 @@ void nlmclnt_release_host(struct nlm_host *host)
>
> WARN_ON_ONCE(host->h_server);
>
> - if (atomic_dec_and_test(&host->h_count)) {
> + if (refcount_dec_and_test(&host->h_count)) {
> WARN_ON_ONCE(!list_empty(&host->h_lockowners));
> WARN_ON_ONCE(!list_empty(&host->h_granted));
> WARN_ON_ONCE(!list_empty(&host->h_reclaim));
> @@ -410,7 +410,7 @@ void nlmsvc_release_host(struct nlm_host *host)
> dprintk("lockd: release server host %s\n", host->h_name);
>
> WARN_ON_ONCE(!host->h_server);
> - atomic_dec(&host->h_count);
> + refcount_dec(&host->h_count);
> }
>
> /*
> @@ -504,7 +504,7 @@ struct nlm_host * nlm_get_host(struct nlm_host *host)
> {
> if (host) {
> dprintk("lockd: get host %s\n", host->h_name);
> - atomic_inc(&host->h_count);
> + refcount_inc(&host->h_count);
> host->h_expires = jiffies + NLM_HOST_EXPIRE;
> }
> return host;
> @@ -593,7 +593,7 @@ static void nlm_complain_hosts(struct net *net)
> if (net && host->net != net)
> continue;
> dprintk(" %s (cnt %d use %d exp %ld net %x)\n",
> - host->h_name, atomic_read(&host->h_count),
> + host->h_name, refcount_read(&host->h_count),
> host->h_inuse, host->h_expires, host->net->ns.inum);
> }
> }
> @@ -662,11 +662,11 @@ nlm_gc_hosts(struct net *net)
> for_each_host_safe(host, next, chain, nlm_server_hosts) {
> if (net && host->net != net)
> continue;
> - if (atomic_read(&host->h_count) || host->h_inuse
> + if (refcount_read(&host->h_count) || host->h_inuse
> || time_before(jiffies, host->h_expires)) {
> dprintk("nlm_gc_hosts skipping %s "
> "(cnt %d use %d exp %ld net %x)\n",
> - host->h_name, atomic_read(&host->h_count),
> + host->h_name, refcount_read(&host->h_count),
> host->h_inuse, host->h_expires,
> host->net->ns.inum);
> continue;
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index d7d313f..39dfeea 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -17,6 +17,7 @@
> #include <net/ipv6.h>
> #include <linux/fs.h>
> #include <linux/kref.h>
> +#include <linux/refcount.h>
> #include <linux/utsname.h>
> #include <linux/lockd/bind.h>
> #include <linux/lockd/xdr.h>
> @@ -58,7 +59,7 @@ struct nlm_host {
> u32 h_state; /* pseudo-state counter */
> u32 h_nsmstate; /* true remote NSM state */
> u32 h_pidcount; /* Pseudopids */
> - atomic_t h_count; /* reference count */
> + refcount_t h_count; /* reference count */
> struct mutex h_mutex; /* mutex for pmap binding */
> unsigned long h_nextrebind; /* next portmap call */
> unsigned long h_expires; /* eligible for GC */
> --
> 2.7.4
On Fri, Dec 22, 2017 at 09:29:15AM +0000, Reshetova, Elena wrote:
>
> On Wed, Nov 29, 2017 at 01:15:43PM +0200, Elena Reshetova wrote:
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> > - counter is initialized to 1 using atomic_set()
> > - a resource is freed upon counter reaching zero
> > - once counter reaches zero, its further
> > increments aren't allowed
> > - counter schema uses basic atomic operations
> > (set, inc, inc_not_zero, dec_and_test, etc.)
>
> >Whoops, I forgot that this doesn't apply to h_count.
>
> >Well, it's confusing, because h_count is actually used in two different
> >ways: depending on whether a nlm_host represents a client or server, it
> >may have the above properties or not.
>
>
> So, what happens when it is not having the above properties? Is the object
> being reused or?
The object isn't destroyed when the counter hits zero--zero is just
taken as a hint to some garbage collection algorithm that it would be OK
to destroy it. So decrementing to or incrementing from zero is OK.
--b.
On Fri, Dec 22, 2017 at 09:25:53AM -0500, J. Bruce Fields wrote:
> On Fri, Dec 22, 2017 at 09:29:15AM +0000, Reshetova, Elena wrote:
> >
> > On Wed, Nov 29, 2017 at 01:15:43PM +0200, Elena Reshetova wrote:
> > > atomic_t variables are currently used to implement reference
> > > counters with the following properties:
> > > - counter is initialized to 1 using atomic_set()
> > > - a resource is freed upon counter reaching zero
> > > - once counter reaches zero, its further
> > > increments aren't allowed
> > > - counter schema uses basic atomic operations
> > > (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > >Whoops, I forgot that this doesn't apply to h_count.
> >
> > >Well, it's confusing, because h_count is actually used in two different
> > >ways: depending on whether a nlm_host represents a client or server, it
> > >may have the above properties or not.
> >
> >
> > So, what happens when it is not having the above properties? Is the object
> > being reused or?
>
> The object isn't destroyed when the counter hits zero--zero is just
> taken as a hint to some garbage collection algorithm that it would be OK
> to destroy it. So decrementing to or incrementing from zero is OK.
In more detail: the nlm_host objects that are used on the NFS server to
represent NFS clients are put by nlmsvc_release_host, and then may
eventually be freed by nlm_gc_hosts.
The nlm_host objects that are used on the NFS client to represent NFS
servers are put (and freed when h_count goes to zero) by
nlmclnt_release_host.
In both cases reference are taken by nlm_get_host. It would be possible
to replace nlm_get_host by two different functions if that would help.
Most callers are obviously only client-side or server-side. The only
exception is next_host_state. It could be passed a pointer to the "get"
function it should use.
After that we might actually just want to define separate client and
server structs like:
struct nlm_clnt_host {
struct nlm_host ch_host;
refcount_t ch_count;
...
}
struct nlm_srv_host {
struct nlm_host sh_host;
refcount_t sh_count;
...
}
rather than have a single h_count which is used in two confusingly
different ways. There are also some other nlm_host fields that really
only make sense for client or server.
--b.
> On Fri, Dec 22, 2017 at 09:25:53AM -0500, J. Bruce Fields wrote:
> > On Fri, Dec 22, 2017 at 09:29:15AM +0000, Reshetova, Elena wrote:
> > >
> > > On Wed, Nov 29, 2017 at 01:15:43PM +0200, Elena Reshetova wrote:
> > > > atomic_t variables are currently used to implement reference
> > > > counters with the following properties:
> > > > - counter is initialized to 1 using atomic_set()
> > > > - a resource is freed upon counter reaching zero
> > > > - once counter reaches zero, its further
> > > > increments aren't allowed
> > > > - counter schema uses basic atomic operations
> > > > (set, inc, inc_not_zero, dec_and_test, etc.)
> > >
> > > >Whoops, I forgot that this doesn't apply to h_count.
> > >
> > > >Well, it's confusing, because h_count is actually used in two different
> > > >ways: depending on whether a nlm_host represents a client or server, it
> > > >may have the above properties or not.
> > >
> > >
> > > So, what happens when it is not having the above properties? Is the object
> > > being reused or?
> >
> > The object isn't destroyed when the counter hits zero--zero is just
> > taken as a hint to some garbage collection algorithm that it would be OK
> > to destroy it. So decrementing to or incrementing from zero is OK.
>
> In more detail: the nlm_host objects that are used on the NFS server to
> represent NFS clients are put by nlmsvc_release_host, and then may
> eventually be freed by nlm_gc_hosts.
>
> The nlm_host objects that are used on the NFS client to represent NFS
> servers are put (and freed when h_count goes to zero) by
> nlmclnt_release_host.
>
> In both cases reference are taken by nlm_get_host. It would be possible
> to replace nlm_get_host by two different functions if that would help.
> Most callers are obviously only client-side or server-side. The only
> exception is next_host_state. It could be passed a pointer to the "get"
> function it should use.
>
> After that we might actually just want to define separate client and
> server structs like:
>
> struct nlm_clnt_host {
> struct nlm_host ch_host;
> refcount_t ch_count;
> ...
> }
>
> struct nlm_srv_host {
> struct nlm_host sh_host;
> refcount_t sh_count;
> ...
> }
>
> rather than have a single h_count which is used in two confusingly
> different ways. There are also some other nlm_host fields that really
> only make sense for client or server.
This sounds reasonable for me, but obviously it is a bigger change and I might not
have enough knowledge on NFS to make it correctly.
In any case, even for the current server case, when freeing might not happen and object gets
re-used later on, is it possible to simply re-initialize the object (and its reference counter) properly before reusing?
I think this is the only thing that is needed from the correct refcounting POV in this case, so
instead of using refcount_inc() on reused object, you would explicitly do refcount_set(counter, 1) when reuse happens.
Best Regards,
Elena
>
> --b.
On Wed, Dec 27, 2017 at 12:10:15PM +0000, Reshetova, Elena wrote:
> > On Fri, Dec 22, 2017 at 09:25:53AM -0500, J. Bruce Fields wrote:
> > > On Fri, Dec 22, 2017 at 09:29:15AM +0000, Reshetova, Elena wrote:
> > > >
> > > > On Wed, Nov 29, 2017 at 01:15:43PM +0200, Elena Reshetova wrote:
> > > > > atomic_t variables are currently used to implement reference
> > > > > counters with the following properties:
> > > > > - counter is initialized to 1 using atomic_set()
> > > > > - a resource is freed upon counter reaching zero
> > > > > - once counter reaches zero, its further
> > > > > increments aren't allowed
> > > > > - counter schema uses basic atomic operations
> > > > > (set, inc, inc_not_zero, dec_and_test, etc.)
> > > >
> > > > >Whoops, I forgot that this doesn't apply to h_count.
> > > >
> > > > >Well, it's confusing, because h_count is actually used in two different
> > > > >ways: depending on whether a nlm_host represents a client or server, it
> > > > >may have the above properties or not.
> > > >
> > > >
> > > > So, what happens when it is not having the above properties? Is the object
> > > > being reused or?
> > >
> > > The object isn't destroyed when the counter hits zero--zero is just
> > > taken as a hint to some garbage collection algorithm that it would be OK
> > > to destroy it. So decrementing to or incrementing from zero is OK.
> >
> > In more detail: the nlm_host objects that are used on the NFS server to
> > represent NFS clients are put by nlmsvc_release_host, and then may
> > eventually be freed by nlm_gc_hosts.
> >
> > The nlm_host objects that are used on the NFS client to represent NFS
> > servers are put (and freed when h_count goes to zero) by
> > nlmclnt_release_host.
> >
> > In both cases reference are taken by nlm_get_host. It would be possible
> > to replace nlm_get_host by two different functions if that would help.
> > Most callers are obviously only client-side or server-side. The only
> > exception is next_host_state. It could be passed a pointer to the "get"
> > function it should use.
> >
> > After that we might actually just want to define separate client and
> > server structs like:
> >
> > struct nlm_clnt_host {
> > struct nlm_host ch_host;
> > refcount_t ch_count;
> > ...
> > }
> >
> > struct nlm_srv_host {
> > struct nlm_host sh_host;
> > refcount_t sh_count;
> > ...
> > }
> >
> > rather than have a single h_count which is used in two confusingly
> > different ways. There are also some other nlm_host fields that really
> > only make sense for client or server.
>
> This sounds reasonable for me, but obviously it is a bigger change and I might not
> have enough knowledge on NFS to make it correctly.
>
> In any case, even for the current server case, when freeing might not happen and object gets
> re-used later on, is it possible to simply re-initialize the object (and its reference counter) properly before reusing?
The object still has useful information in it so we can't just
reinitalize it completely. I guess we could make nlm_get_host do
if (refcount_read(&host->h_count))
refcount_inc(&host->h_count);
else
refcount_set(&host->h_count, 1);
Or we could just change the code so the refcount is always 1 higher in
the NFS server case, so "1" instead of "0" is used to mean "nobody's
using this, you can garbage collect this", and then it won't go to 0
until the garbage collector actually destroys it.
This isn't an unusual pattern, what have other subsystems been doing?
--b.
> I think this is the only thing that is needed from the correct refcounting POV in this case, so
> instead of using refcount_inc() on reused object, you would explicitly do refcount_set(counter, 1) when reuse happens.
On Tue, 2018-01-23 at 17:09 -0500, J. Bruce Fields wrote:
>
> The object still has useful information in it so we can't just
> reinitalize it completely. I guess we could make nlm_get_host do
>
> if (refcount_read(&host->h_count))
> refcount_inc(&host->h_count);
> else
> refcount_set(&host->h_count, 1);
>
> Or we could just change the code so the refcount is always 1 higher
> in
> the NFS server case, so "1" instead of "0" is used to mean "nobody's
> using this, you can garbage collect this", and then it won't go to 0
> until the garbage collector actually destroys it.
>
> This isn't an unusual pattern, what have other subsystems been doing?
>
Hi Bruce,
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.
What say we fix the issue with something like the following?
8<------------------------------------------------------------
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
> [email protected]