[PATCH] NFS: fix bug in legacy DNS resolver.

[PATCH] NFS: fix bug in legacy DNS resolver.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]


The DNS resolver's use of the sunrpc cache involves a 'ttl' number
(relative) rather that a timeout (absolute).  This confused me when
I wrote
  commit c5b29f885afe890f953f7f23424045cdad31d3e4
     "sunrpc: use seconds since boot in expiry cache"

and I managed to break it.  The effect is that any TTL is interpreted
as 0, and nothing useful gets into the cache.

This patch removes the use of get_expiry() - which really expects an
expiry time - and uses get_uint() instead, treating the int correctly
as a ttl.

This fixes a regression that has been present since 2.6.37, causing
certain NFS accesses in certain environments to incorrectly fail.

Reported-by: Chuck Lever <chuck.lever@oracle.com>
Tested-by: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/fs/nfs/dns_resolve.c b/fs/nfs/dns_resolve.c
index 31c26c4..d9415a2 100644
--- a/fs/nfs/dns_resolve.c
+++ b/fs/nfs/dns_resolve.c
@@ -217,7 +217,7 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
 {
 	char buf1[NFS_DNS_HOSTNAME_MAXLEN+1];
 	struct nfs_dns_ent key, *item;
-	unsigned long ttl;
+	unsigned int ttl;
 	ssize_t len;
 	int ret = -EINVAL;
 
@@ -240,7 +240,8 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
 	key.namelen = len;
 	memset(&key.h, 0, sizeof(key.h));
 
-	ttl = get_expiry(&buf);
+	if (get_int(&buf, &ttl) < 0)
+		goto out;
 	if (ttl == 0)
 		goto out;
 	key.h.expiry_time = ttl + seconds_since_boot();

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] NFS: fix bug in legacy DNS resolver.

[Chuck Lever]

Thanks, Neil!

On Oct 30, 2012, at 9:16 PM, NeilBrown <neilb@suse.de> wrote:

> 
> The DNS resolver's use of the sunrpc cache involves a 'ttl' number
> (relative) rather that a timeout (absolute).  This confused me when
> I wrote
>  commit c5b29f885afe890f953f7f23424045cdad31d3e4
>     "sunrpc: use seconds since boot in expiry cache"
> 
> and I managed to break it.  The effect is that any TTL is interpreted
> as 0, and nothing useful gets into the cache.
> 
> This patch removes the use of get_expiry() - which really expects an
> expiry time - and uses get_uint() instead, treating the int correctly
> as a ttl.

Nit: The description says "get_uint()" but the patch uses "get_int()."  Should we change the patch?

> This fixes a regression that has been present since 2.6.37, causing
> certain NFS accesses in certain environments to incorrectly fail.

In particular, when the legacy DNS resolver is in use, this regression prevents the NFS client from following any NFSv4 referral which contains a DNS hostname.

> Reported-by: Chuck Lever <chuck.lever@oracle.com>
> Tested-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/fs/nfs/dns_resolve.c b/fs/nfs/dns_resolve.c
> index 31c26c4..d9415a2 100644
> --- a/fs/nfs/dns_resolve.c
> +++ b/fs/nfs/dns_resolve.c
> @@ -217,7 +217,7 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
> {
> 	char buf1[NFS_DNS_HOSTNAME_MAXLEN+1];
> 	struct nfs_dns_ent key, *item;
> -	unsigned long ttl;
> +	unsigned int ttl;
> 	ssize_t len;
> 	int ret = -EINVAL;
> 
> @@ -240,7 +240,8 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
> 	key.namelen = len;
> 	memset(&key.h, 0, sizeof(key.h));
> 
> -	ttl = get_expiry(&buf);
> +	if (get_int(&buf, &ttl) < 0)
> +		goto out;
> 	if (ttl == 0)
> 		goto out;
> 	key.h.expiry_time = ttl + seconds_since_boot();

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] NFS: fix bug in legacy DNS resolver.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Wed, 31 Oct 2012 11:09:27 -0400 Chuck Lever <chuck.lever@oracle.com> wrote:

> Thanks, Neil!
> 
> On Oct 30, 2012, at 9:16 PM, NeilBrown <neilb@suse.de> wrote:
> 
> > 
> > The DNS resolver's use of the sunrpc cache involves a 'ttl' number
> > (relative) rather that a timeout (absolute).  This confused me when
> > I wrote
> >  commit c5b29f885afe890f953f7f23424045cdad31d3e4
> >     "sunrpc: use seconds since boot in expiry cache"
> > 
> > and I managed to break it.  The effect is that any TTL is interpreted
> > as 0, and nothing useful gets into the cache.
> > 
> > This patch removes the use of get_expiry() - which really expects an
> > expiry time - and uses get_uint() instead, treating the int correctly
> > as a ttl.
> 
> Nit: The description says "get_uint()" but the patch uses "get_int()."  Should we change the patch?
> 

Yes, change the patch.  I remember looking at that issue and deciding that
get_uint() was the right thing to use as 'ttl' is already unsigned.
And I wrote the description based on the memory.  But it seems I didn't
change the code.  Weird.

> > This fixes a regression that has been present since 2.6.37, causing
> > certain NFS accesses in certain environments to incorrectly fail.
> 
> In particular, when the legacy DNS resolver is in use, this regression prevents the NFS client from following any NFSv4 referral which contains a DNS hostname.

Thanks.  Updated patch below.

NeilBrown



From e0b382c5b880017853f6a4cc0eda472cf6fef0e4 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Wed, 31 Oct 2012 12:10:48 +1100
Subject: [PATCH] NFS: fix bug in legacy DNS resolver.

The DNS resolver's use of the sunrpc cache involves a 'ttl' number
(relative) rather that a timeout (absolute).  This confused me when
I wrote
  commit c5b29f885afe890f953f7f23424045cdad31d3e4
     "sunrpc: use seconds since boot in expiry cache"

and I managed to break it.  The effect is that any TTL is interpreted
as 0, and nothing useful gets into the cache.

This patch removes the use of get_expiry() - which really expects an
expiry time - and uses get_uint() instead, treating the int correctly
as a ttl.

This fixes a regression that has been present since 2.6.37, causing
certain NFS accesses in certain environments to incorrectly fail.

In particular, when the legacy DNS resolver is in use, this regression
prevents the NFS client from following any NFSv4 referral which
contains a DNS hostname.

Reported-by: Chuck Lever <chuck.lever@oracle.com>
Tested-by: Chuck Lever <chuck.lever@oracle.com>
Cc: stable@vger.kernel.org
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/fs/nfs/dns_resolve.c b/fs/nfs/dns_resolve.c
index 31c26c4..ca4b11e 100644
--- a/fs/nfs/dns_resolve.c
+++ b/fs/nfs/dns_resolve.c
@@ -217,7 +217,7 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
 {
 	char buf1[NFS_DNS_HOSTNAME_MAXLEN+1];
 	struct nfs_dns_ent key, *item;
-	unsigned long ttl;
+	unsigned int ttl;
 	ssize_t len;
 	int ret = -EINVAL;
 
@@ -240,7 +240,8 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
 	key.namelen = len;
 	memset(&key.h, 0, sizeof(key.h));
 
-	ttl = get_expiry(&buf);
+	if (get_uint(&buf, &ttl) < 0)
+		goto out;
 	if (ttl == 0)
 		goto out;
 	key.h.expiry_time = ttl + seconds_since_boot();



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]