[PATCH 0/2] autofs: fix two related bugs with lookup + expired yp maps

[PATCH 0/2] autofs: fix two related bugs with lookup + expired yp maps

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

Hi Ian -

We recently encountered an issue where a client would all of a sudden
start returning ELOOP for anything involving crossing an autofs mount.
We tracked it down to two bugs, one which obscured the other.  The first
is that we were not properly updating the map age for yp maps when we retry
the lookup with dots instead of underscores.  The second is that it is
possible under several situations to return success when there was a
failure in lookup_nss_mount.  In this particular case, it was the only
map in the list and since the age wasn't update, it was skipped.  Then
we exit the while loop with ret == 0, ultimately returning success to
the caller.  Autofs would tell the kernel it succeeded, the kernel would
retry the lookup, and we'd loop until we hit the kernel loop limit (40).

These two patches fix each of the issues.

Thanks,

-Jeff

Jeff Mahoney (2):
  autofs: fix yp map age not updated in s/_/./g case
  autofs: properly handle errors in lookup_nss_mount

 daemon/lookup.c     |  5 ++++-
 modules/lookup_yp.c | 17 ++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.7.1

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

[PATCH 1/2] autofs: fix yp map age not updated in s/_/./g case

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

The map source age field should be updated when we retry
the yp map lookup using dots instead of underscores.

Rather than just open code the exit path, just reverse the logic for
returning success so we only have one success path for both cases.
---
 modules/lookup_yp.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/modules/lookup_yp.c b/modules/lookup_yp.c
index eda2aba..502d850 100644
--- a/modules/lookup_yp.c
+++ b/modules/lookup_yp.c
@@ -431,17 +431,16 @@ int lookup_read_map(struct autofs_point *ap, time_t age, void *context)
 			err = yp_all((char *) ctxt->domainname, mapname, &ypcb);
 		}
 
-		if (err == YPERR_SUCCESS)
-			return NSS_STATUS_SUCCESS;
+		if (err != YPERR_SUCCESS) {
+			warn(ap->logopt,
+			     MODPREFIX "read of map %s failed: %s",
+			     ap->path, yperr_string(err));
 
-		warn(ap->logopt,
-		     MODPREFIX "read of map %s failed: %s",
-		     ap->path, yperr_string(err));
+			if (err == YPERR_PMAP || err == YPERR_YPSERV)
+				return NSS_STATUS_UNAVAIL;
 
-		if (err == YPERR_PMAP || err == YPERR_YPSERV)
-			return NSS_STATUS_UNAVAIL;
-
-		return NSS_STATUS_NOTFOUND;
+			return NSS_STATUS_NOTFOUND;
+		}
 	}
 
 	source->age = age;
-- 
2.7.1

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

[PATCH 2/2] autofs: properly handle errors in lookup_nss_mount

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

The 'result' variable is initialized to 0 and inconsistently
set in the main while loop.  If the map is expired, the strdup("ldap")
fails, or we end up bailing on an sss source with an amd map, and we're
processing the only map in the list or check_nss_result indicates that
we should continue even when we have NSS_STATUS_SUCCESS, we'll return
success when a failure has occured.

Without this change, the end result is still a failure for the user.
The caller attempting to traverse the map will eventually receive
ELOOP once the kernel has reached its internal automount/link follow
limit.  We will have just wasted a bunch of time in the interim.  This
behavior was observed on a system that was affected by the missing
map->age update fixed in the previous patch.
---
 daemon/lookup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemon/lookup.c b/daemon/lookup.c
index 0129f75..173f53e 100644
--- a/daemon/lookup.c
+++ b/daemon/lookup.c
@@ -1073,7 +1073,7 @@ int lookup_nss_mount(struct autofs_point *ap, struct map_source *source, const c
 	struct nss_source *this;
 	struct map_source *map;
 	enum nsswitch_status status;
-	int result = 0;
+	int result = NSS_STATUS_UNKNOWN;
 
 	/*
 	 * For each map source (ie. each entry for the mount
@@ -1092,6 +1092,7 @@ int lookup_nss_mount(struct autofs_point *ap, struct map_source *source, const c
 		 * the map entry was last updated.
 		 */
 		if (entry->age > map->age) {
+			status = NSS_STATUS_UNAVAIL;
 			map = map->next;
 			continue;
 		}
@@ -1113,6 +1114,7 @@ int lookup_nss_mount(struct autofs_point *ap, struct map_source *source, const c
 				char *tmp = strdup("ldap");
 				if (!tmp) {
 					map = map->next;
+					status = NSS_STATUS_TRYAGAIN;
 					continue;
 				}
 				map->type = tmp;
@@ -1145,6 +1147,7 @@ int lookup_nss_mount(struct autofs_point *ap, struct map_source *source, const c
 			    !strcmp(this->source, "sss")) {
 				warn(ap->logopt,
 				     "source sss is not available for amd maps.");
+				result = NSS_STATUS_UNAVAIL;
 				continue;
 			}
 
-- 
2.7.1

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH 0/2] autofs: fix two related bugs with lookup + expired yp maps

[Ian Kent]

On Wed, 2016-05-04 at 20:03 -0400, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> Hi Ian -
> 
> We recently encountered an issue where a client would all of a sudden
> start returning ELOOP for anything involving crossing an autofs mount.
> We tracked it down to two bugs, one which obscured the other.  The
> first
> is that we were not properly updating the map age for yp maps when we
> retry
> the lookup with dots instead of underscores.  The second is that it is
> possible under several situations to return success when there was a
> failure in lookup_nss_mount.  In this particular case, it was the only
> map in the list and since the age wasn't update, it was skipped.  Then
> we exit the while loop with ret == 0, ultimately returning success to
> the caller.  Autofs would tell the kernel it succeeded, the kernel
> would
> retry the lookup, and we'd loop until we hit the kernel loop limit
> (40).
> 
> These two patches fix each of the issues.

OK, that sounds good.

I'll have a look at the code to make sure I understand what's going on
before adding these to the list of patches I have.

That list is getting a bit large now so I'll likely be committing them
soonish but I can't say yet when I'll release 5.1.2.

Up until now I thought that the source of the ELOOP returns that some
people have seen were due to incorrect returns of in kernel mounted
checks where mounts were present in other namespaces.

My most recent attempt at resolving that had a positive response from a
tester here on the list but another tester experienced file system
corruption. I'm totally at mystified by this so I'm a bit stuck at the
moment. 

> 
> Thanks,
> 
> -Jeff
> 
> Jeff Mahoney (2):
>   autofs: fix yp map age not updated in s/_/./g case
>   autofs: properly handle errors in lookup_nss_mount
> 
>  daemon/lookup.c     |  5 ++++-
>  modules/lookup_yp.c | 17 ++++++++---------
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH 0/2] autofs: fix two related bugs with lookup + expired yp maps

[Ian Kent]

On Thu, 2016-05-05 at 16:35 +0800, Ian Kent wrote:
> On Wed, 2016-05-04 at 20:03 -0400, jeffm@suse.com wrote:
> > From: Jeff Mahoney <jeffm@suse.com>
> > 
> > Hi Ian -
> > 
> > We recently encountered an issue where a client would all of a sudden
> > start returning ELOOP for anything involving crossing an autofs mount.
> > We tracked it down to two bugs, one which obscured the other.  The
> > first
> > is that we were not properly updating the map age for yp maps when we
> > retry
> > the lookup with dots instead of underscores.  The second is that it is
> > possible under several situations to return success when there was a
> > failure in lookup_nss_mount.  In this particular case, it was the only
> > map in the list and since the age wasn't update, it was skipped.  Then
> > we exit the while loop with ret == 0, ultimately returning success to
> > the caller.  Autofs would tell the kernel it succeeded, the kernel
> > would
> > retry the lookup, and we'd loop until we hit the kernel loop limit
> > (40).
> > 
> > These two patches fix each of the issues.
> 
> OK, that sounds good.
> 
> I'll have a look at the code to make sure I understand what's going on
> before adding these to the list of patches I have.
> 
> That list is getting a bit large now so I'll likely be committing them
> soonish but I can't say yet when I'll release 5.1.2.
> 
> Up until now I thought that the source of the ELOOP returns that some
> people have seen were due to incorrect returns of in kernel mounted
> checks where mounts were present in other namespaces.
> 
> My most recent attempt at resolving that had a positive response from a
> tester here on the list but another tester experienced file system
> corruption. I'm totally at mystified by this so I'm a bit stuck at the
> moment. 

When importing these I've made a couple of changes I hope you will be ok with.
The content of the patches remains the same (assuming I have no other questions
when I properly review them).

Due to my convention of trying to always have the patch title the same as the
patch name I had to change the first patch.

I changed the patch name (and title with "-" changed to " ") to:
autofs-5.1.1-fix-yp-map-age-not-updated-during-map-lookup.patch

The description remains the same and I think still conveys the intent of the
change.

I changed only autofs: to autofs-5.1.1 in the second patch.

Finally I assume your ok with me adding your "Signed-off-by:" to both patches,
along with mine.

> 
> > 
> > Thanks,
> > 
> > -Jeff
> > 
> > Jeff Mahoney (2):
> >   autofs: fix yp map age not updated in s/_/./g case
> >   autofs: properly handle errors in lookup_nss_mount
> > 
> >  daemon/lookup.c     |  5 ++++-
> >  modules/lookup_yp.c | 17 ++++++++---------
> >  2 files changed, 12 insertions(+), 10 deletions(-)
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe autofs" in
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH 0/2] autofs: fix two related bugs with lookup + expired yp maps

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2702 bytes --]

On 5/5/16 6:21 AM, Ian Kent wrote:
> On Thu, 2016-05-05 at 16:35 +0800, Ian Kent wrote:
>> On Wed, 2016-05-04 at 20:03 -0400, jeffm@suse.com wrote:
>>> From: Jeff Mahoney <jeffm@suse.com>
>>>
>>> Hi Ian -
>>>
>>> We recently encountered an issue where a client would all of a sudden
>>> start returning ELOOP for anything involving crossing an autofs mount.
>>> We tracked it down to two bugs, one which obscured the other.  The
>>> first
>>> is that we were not properly updating the map age for yp maps when we
>>> retry
>>> the lookup with dots instead of underscores.  The second is that it is
>>> possible under several situations to return success when there was a
>>> failure in lookup_nss_mount.  In this particular case, it was the only
>>> map in the list and since the age wasn't update, it was skipped.  Then
>>> we exit the while loop with ret == 0, ultimately returning success to
>>> the caller.  Autofs would tell the kernel it succeeded, the kernel
>>> would
>>> retry the lookup, and we'd loop until we hit the kernel loop limit
>>> (40).
>>>
>>> These two patches fix each of the issues.
>>
>> OK, that sounds good.
>>
>> I'll have a look at the code to make sure I understand what's going on
>> before adding these to the list of patches I have.
>>
>> That list is getting a bit large now so I'll likely be committing them
>> soonish but I can't say yet when I'll release 5.1.2.
>>
>> Up until now I thought that the source of the ELOOP returns that some
>> people have seen were due to incorrect returns of in kernel mounted
>> checks where mounts were present in other namespaces.
>>
>> My most recent attempt at resolving that had a positive response from a
>> tester here on the list but another tester experienced file system
>> corruption. I'm totally at mystified by this so I'm a bit stuck at the
>> moment. 
> 
> When importing these I've made a couple of changes I hope you will be ok with.
> The content of the patches remains the same (assuming I have no other questions
> when I properly review them).
> 
> Due to my convention of trying to always have the patch title the same as the
> patch name I had to change the first patch.
> 
> I changed the patch name (and title with "-" changed to " ") to:
> autofs-5.1.1-fix-yp-map-age-not-updated-during-map-lookup.patch
> 
> The description remains the same and I think still conveys the intent of the
> change.
> 
> I changed only autofs: to autofs-5.1.1 in the second patch.
> 
> Finally I assume your ok with me adding your "Signed-off-by:" to both patches,
> along with mine.

Yep, sounds good to me.

Thanks,

-Jeff


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]