Re: [PATCH 0/3] Fix use_ipaddr race

[NeilBrown <neilb@suse.de>]

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

On Fri, 20 Apr 2012 18:46:15 -0400 "J. Bruce Fields" <bfields@redhat.com>
wrote:

> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> We have a report of failed upcalls that occur when use_ipaddr is
> toggled.  The problem appears to be that for example after turning on
> use_ipaddr, the kernel may still see upcalls for clients such as "*".
> 
> The following patches (untested except to check that they compile...)
> attempt to fix that by just letting nfsd_fh() accept either client type;
> does anyone see a problem with that?
> 
> 
> The current code actually attempts to avoid tihs problem by flushing
> caches on a use_ipaddr change (see the cache_flush() call in
> utils/mountd/auth.c:check_useipaddr().  But that doesn't work, because a
> write to a cache/flush file doesn't actually provide useful guarantees
> to the caller on return:
> 
> 	- as far as I can tell, cache_clean leaves alone any entries
> 	  that were created in the current second.

We only do one thing each second - right?

Maybe a simple work around is:
  - update state in mountd
  - sleep(1)
  - flush cache.


?? Then anything dependant on the old behaviour will be gone.

> 	- cache_clean only removes entries from the cache, it doesn't
> 	  wait for them to actually be destroyed: so an in-progress nfsd
> 	  thread could still make an upcall using information from one
> 	  of the flushed entries.

Ideally we would like those threads to abort and re-start from the beginning.
That's not really very easy with all the NFSv4 state issues is it?

I think waiting for them to complete would be problematic.  As you say,
deadlocks and such.
So we need to continue to serve them faithfully.

Does that just mean changing nfsd_fh to *not* test use_ipaddr, but rather to
inspect the 'dom' and if it looks like an IP address, act as though use_ipaddr
was set, else not?

> 
> These seem like bugs in their own right to me: a cache-flush operation
> that actually guaranteed the entries were gone on return would be more
> useful.  And I wonder whether this doesn't also cause exportfs bugs....
> 
> I'm not sure what to do about it, though.
> 
> I don't think the existing interface is really fixable, since fixing the
> second problem above would (I think) require the cache/flush write to
> wait on in-progress rpc's, but those in-progress rpc's could be waiting
> on mountd, creating a deadlock.
> 
> A new interface shouldn't need to accept a time--every actual user just
> wants to cache flushed now.

Having a time-stamp seems like a good idea at the time .... I don't remember
why though. :-(

> 
> Maybe the first problem could be solved by replacing the time by a
> counter incremented on each insert of a cache entry.
> 
> And the second could be fixed by waiting on in-progress rpc's.  That
> might not help mountd, but it would help exportfs at least.
> 
> --b.
> 
> J. Bruce Fields (3):
>   mountd: unconditionally resolve ip address

Not a good idea.  If /etc/exports only contains IP address and subnets, then
we don't ever want to do any address resolution.  The "resolve ip address"
must at least be conditional on "are there any domain-name, wild-cards,
netgroups in /etc/exports".


>   mountd: helper function for export upcall's client matching

This depends on the earlier patch?  and does the host-name lookup earlier.
That really can cause a delay sometimes so it should be as late as possible.


>   mountd: ignore use_ipaddr and just try both client types

Maybe ... though if we could syntactically distinguish "use_ipaddr" domains
from "!use_ipaddr" domain and still just choose one test to perform, I think
I'd prefer that.


NeilBrown

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