What's a good default TTL for DNS keys in the kernel

What's a good default TTL for DNS keys in the kernel

[David Howells]

Since key.dns_resolver isn't given a TTL for the address information obtained
for getaddrinfo(), no expiry is set on dns_resolver keys in the kernel for
NFS, CIFS or Ceph.  AFS gets one if it looks up a cell SRV or AFSDB record
because that is looked up in the DNS directly, but it doesn't look up A or
AAAA records, so doesn't get an expiry for the addresses themselves.

I've previously asked the libc folks if there's a way to get this information
exposed in struct addrinfo, but I don't think that ended up going anywhere -
and, in any case, would take a few years to work through the system.

For the moment, I think I should put a default on any dns_resolver keys and
have it applied either by the kernel (configurable with a /proc/sys/ setting)
or by the key.dnf_resolver program (configurable with an /etc file).

Any suggestion as to the preferred default TTL?  10 minutes?

David

Re: What's a good default TTL for DNS keys in the kernel

[Jeff Layton]

On Tue, 2020-04-14 at 15:20 +0100, David Howells wrote:
> Since key.dns_resolver isn't given a TTL for the address information obtained
> for getaddrinfo(), no expiry is set on dns_resolver keys in the kernel for
> NFS, CIFS or Ceph.  AFS gets one if it looks up a cell SRV or AFSDB record
> because that is looked up in the DNS directly, but it doesn't look up A or
> AAAA records, so doesn't get an expiry for the addresses themselves.
> 
> I've previously asked the libc folks if there's a way to get this information
> exposed in struct addrinfo, but I don't think that ended up going anywhere -
> and, in any case, would take a few years to work through the system.
> 
> For the moment, I think I should put a default on any dns_resolver keys and
> have it applied either by the kernel (configurable with a /proc/sys/ setting)
> or by the key.dnf_resolver program (configurable with an /etc file).
> 
> Any suggestion as to the preferred default TTL?  10 minutes?
> 

Typical DNS TTL values are on the order of a day but it can vary widely.
There's really no correct answer for this, since you have no way to tell
how long the entry has been sitting in the DNS server's cache before you
queried for it.

So, you're probably down to just finding some value that doesn't hammer
the DNS server too much, but that allows you to get new entries in a
reasonable amount of time.

10 mins sounds like a reasonable default to me.
-- 
Jeff Layton <jlayton@kernel.org>

Re: What's a good default TTL for DNS keys in the kernel

[Steve French]

On Wed, Apr 15, 2020 at 8:22 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2020-04-14 at 15:20 +0100, David Howells wrote:
> > Since key.dns_resolver isn't given a TTL for the address information obtained
> > for getaddrinfo(), no expiry is set on dns_resolver keys in the kernel for
> > NFS, CIFS or Ceph.  AFS gets one if it looks up a cell SRV or AFSDB record
> > because that is looked up in the DNS directly, but it doesn't look up A or
> > AAAA records, so doesn't get an expiry for the addresses themselves.
> >
> > I've previously asked the libc folks if there's a way to get this information
> > exposed in struct addrinfo, but I don't think that ended up going anywhere -
> > and, in any case, would take a few years to work through the system.
> >
> > For the moment, I think I should put a default on any dns_resolver keys and
> > have it applied either by the kernel (configurable with a /proc/sys/ setting)
> > or by the key.dnf_resolver program (configurable with an /etc file).
> >
> > Any suggestion as to the preferred default TTL?  10 minutes?
> >
>
> Typical DNS TTL values are on the order of a day but it can vary widely.
> There's really no correct answer for this, since you have no way to tell
> how long the entry has been sitting in the DNS server's cache before you
> queried for it.
>
> So, you're probably down to just finding some value that doesn't hammer
> the DNS server too much, but that allows you to get new entries in a
> reasonable amount of time.
>
> 10 mins sounds like a reasonable default to me.

I would lean toward slightly longer (20 minutes?) but aren't there
usually different timeouts for 'static' vs. 'dynamic' DNS records (so
static records would have longer timeouts)?


-- 
Thanks,

Steve

Re: What's a good default TTL for DNS keys in the kernel

[David Howells]

Steve French <smfrench@gmail.com> wrote:

> > 10 mins sounds like a reasonable default to me.
> 
> I would lean toward slightly longer (20 minutes?) but aren't there
> usually different timeouts for 'static' vs. 'dynamic' DNS records (so
> static records would have longer timeouts)?

Unfortunately, getaddrinfo() doesn't give me that information.

David

Re: What's a good default TTL for DNS keys in the kernel

[Florian Weimer]

* David Howells:

> Since key.dns_resolver isn't given a TTL for the address information obtained
> for getaddrinfo(), no expiry is set on dns_resolver keys in the kernel for
> NFS, CIFS or Ceph.  AFS gets one if it looks up a cell SRV or AFSDB record
> because that is looked up in the DNS directly, but it doesn't look up A or
> AAAA records, so doesn't get an expiry for the addresses themselves.
>
> I've previously asked the libc folks if there's a way to get this information
> exposed in struct addrinfo, but I don't think that ended up going anywhere -
> and, in any case, would take a few years to work through the system.
>
> For the moment, I think I should put a default on any dns_resolver keys and
> have it applied either by the kernel (configurable with a /proc/sys/ setting)
> or by the key.dnf_resolver program (configurable with an /etc file).
>
> Any suggestion as to the preferred default TTL?  10 minutes?

You can get the real TTL if you do a DNS resolution on the name and
match the addresses against what you get out of the NSS functions.  If
they match, you can use the TTL from DNS.  Hackish, but it does give you
*some* TTL value.

The question remains what the expected impact of TTL expiry is.  Will
the kernel just perform a new DNS query if it needs one?  Or would you
expect that (say) the NFS client rechecks the addresses after TTL expiry
and if they change, reconnect to a new NFS server?

If a TTL expiration does not trigger anything, than it seems purely an
optimization to avoid kernel → userspace callbacks.  I think you can do
with a very short TTL in this case, on the order of seconds (or no
caching at all).

Negative caching is also worthy of consideration and can be considerably
more tricky.

Thanks,
Florian

Re: What's a good default TTL for DNS keys in the kernel

[David Howells]

Florian Weimer <fweimer@redhat.com> wrote:

> You can get the real TTL if you do a DNS resolution on the name and
> match the addresses against what you get out of the NSS functions.  If
> they match, you can use the TTL from DNS.  Hackish, but it does give you
> *some* TTL value.

I guess I'd have to do that in parallel.  Would calling something like
res_mkquery() use local DNS caching?

> The question remains what the expected impact of TTL expiry is.  Will
> the kernel just perform a new DNS query if it needs one?  Or would you
> expect that (say) the NFS client rechecks the addresses after TTL expiry
> and if they change, reconnect to a new NFS server?

It depends on the filesystem.

AFS keeps track of the expiration on the record and will issue a new lookup
when the data expires, but NFS doesn't make use of this information.  The
keyring subsystem will itself dispose of dns_resolver keys that expire and
request_key() will only upcall again if the key has expired.

The problem for NFS is that the host IP address is the primary key for the
superblock (see nfs_compare_super_address()).

CIFS also doesn't make direct use of the TTL, and again this may be because it
uses the server address as part of the primary key for the superblock (see
cifs_match_super()).

David

Re: What's a good default TTL for DNS keys in the kernel

[Florian Weimer]

* David Howells:

> Florian Weimer <fweimer@redhat.com> wrote:
>
>> You can get the real TTL if you do a DNS resolution on the name and
>> match the addresses against what you get out of the NSS functions.  If
>> they match, you can use the TTL from DNS.  Hackish, but it does give you
>> *some* TTL value.
>
> I guess I'd have to do that in parallel.

Not necessary.  You can do the getaddrinfo lookup first and then perform
the query.

> Would calling something like res_mkquery() use local DNS caching?

Yes (but res_mkquery builds a packet, it does not send it).

>> The question remains what the expected impact of TTL expiry is.  Will
>> the kernel just perform a new DNS query if it needs one?  Or would you
>> expect that (say) the NFS client rechecks the addresses after TTL expiry
>> and if they change, reconnect to a new NFS server?
>
> It depends on the filesystem.
>
> AFS keeps track of the expiration on the record and will issue a new lookup
> when the data expires, but NFS doesn't make use of this information.

And it will switch servers at that point?  Or only if the existing
server association fails/times out?

> The keyring subsystem will itself dispose of dns_resolver keys that
> expire and request_key() will only upcall again if the key has
> expired.

What's are higher-level effects of that?

I'm still not convinced that the kernel *needs* accurate TTL
information.  The benefit from upcall avoidance likely vanishes quickly
after the in-kernel TTL increases beyond 5 or so.  That's just my guess,
though.

Thanks,
Florian

Re: What's a good default TTL for DNS keys in the kernel

[David Howells]

Florian Weimer <fweimer@redhat.com> wrote:

> > Florian Weimer <fweimer@redhat.com> wrote:
> >
> >> You can get the real TTL if you do a DNS resolution on the name and
> >> match the addresses against what you get out of the NSS functions.  If
> >> they match, you can use the TTL from DNS.  Hackish, but it does give you
> >> *some* TTL value.
> >
> > I guess I'd have to do that in parallel.
> 
> Not necessary.  You can do the getaddrinfo lookup first and then perform
> the query.

That means that the latency of both is added together and causes the first
mount to take longer - though as long as you have a local DNS cache, that's
fine.

> > AFS keeps track of the expiration on the record and will issue a new lookup
> > when the data expires, but NFS doesn't make use of this information.
> 
> And it will switch servers at that point?  Or only if the existing
> server association fails/times out?

AFS will switch servers at the next operation if the server list changes.  And
if the current op tries to access an old server and gets bounced, this should
trigger an immediate reevaluation.  It also regularly probes the servers and
interfaces it knows about to find which one's accessible and which has the
best response and can switch servers on that basis also.

I should also note that AFS deletes the dns_resolver key after reading it and
maintains the expiry information in its internal structs.

Note also that in AFS this only applies to locating the Volume Location
servers (which is a layer of abstraction that hides which server(s) a volume
resides on and what their addresses are).  The VL service is queried to find
out where file servers are (giving you their addresses itself so you don't
need to access the DNS there).

> > The keyring subsystem will itself dispose of dns_resolver keys that
> > expire and request_key() will only upcall again if the key has
> > expired.
> 
> What's are higher-level effects of that?

If the record never expires (the current case), the address lookup in the
kernel (dns_query()) will always return the same address until someone
manually evicts it.

Otherwise, once the record expires, the kernel will just upcall again.

> I'm still not convinced that the kernel *needs* accurate TTL
> information.  The benefit from upcall avoidance likely vanishes quickly
> after the in-kernel TTL increases beyond 5 or so.  That's just my guess,
> though.

You might be right - certainly for NFS and CIFS where the address ascribed to
a superblock is hard to change as it partly defines the superblock.  Change
the address and your superblock in now a different thing as far as the VFS is
concerned.

This makes fscache indexing tricky for NFS.  How do you define a superblock?
Is it address?  Is it hostname?  What happens if one or the other changes?
What happens if there are two or more addresses (say ipv4 and ipv6 addrs)?

AFS defined some abstractions for this: the cell name and the volume ID
number.  The physical location of the volume doesn't matter - and the volume
can even be moved around whilst in use.

David

Re: What's a good default TTL for DNS keys in the kernel

[Chuck Lever]

Hi David-

> On Apr 16, 2020, at 6:27 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Florian Weimer <fweimer@redhat.com> wrote:
> 
>> You can get the real TTL if you do a DNS resolution on the name and
>> match the addresses against what you get out of the NSS functions.  If
>> they match, you can use the TTL from DNS.  Hackish, but it does give you
>> *some* TTL value.
> 
> I guess I'd have to do that in parallel.  Would calling something like
> res_mkquery() use local DNS caching?
> 
>> The question remains what the expected impact of TTL expiry is.  Will
>> the kernel just perform a new DNS query if it needs one?  Or would you
>> expect that (say) the NFS client rechecks the addresses after TTL expiry
>> and if they change, reconnect to a new NFS server?
> 
> It depends on the filesystem.

Agreed. For example:

The Linux NFS client won't connect to a new server when the server's
DNS information changes. A fresh mount operation would be needed for
the client to recognize and make use of it.

There are mechanisms in the NFSv4 protocol to collect server IP addresses
from the server itself (fs_locations) and then try those locations if the
current server fails to respond. But currently that is not implemented in
Linux (and servers would need to be ready to provide that kind of update).


> AFS keeps track of the expiration on the record and will issue a new lookup
> when the data expires, but NFS doesn't make use of this information.  The
> keyring subsystem will itself dispose of dns_resolver keys that expire and
> request_key() will only upcall again if the key has expired.

Our NFS colleagues working on Solaris have noted that handling the expiry
of DNS information can be tricky. It is usually desirable to continue using
expired information when a new DNS query fails temporarily (times out, or
the network is partitioned, etc). That makes for a more robust network file
service.


> The problem for NFS is that the host IP address is the primary key for the
> superblock (see nfs_compare_super_address()).

I thought that NFSv4.1 and newer have server-provided unique information
that might be used in place of the server's IP address. This information
is supposed to be independent of a server's network addresses.


> CIFS also doesn't make direct use of the TTL, and again this may be because it
> uses the server address as part of the primary key for the superblock (see
> cifs_match_super()).

--
Chuck Lever

Re: What's a good default TTL for DNS keys in the kernel

[Aurélien Aptel]

Chuck Lever <chuck.lever@oracle.com> writes:
> The Linux NFS client won't connect to a new server when the server's
> DNS information changes. A fresh mount operation would be needed for
> the client to recognize and make use of it.
>
> There are mechanisms in the NFSv4 protocol to collect server IP addresses
> from the server itself (fs_locations) and then try those locations if the
> current server fails to respond. But currently that is not implemented in
> Linux (and servers would need to be ready to provide that kind of update).

We have a very similar system in CIFS. Failover can be handled in 2 ways
(technically both can be used at the same time):

a) with DFS, the mount can have a list of possible location to connect
   to, sort of like cross-server symlinks with multiple possible
   targets. Note that the target value uses hostnames.
b) the domain controler can notice the server is down and automatically
   switch the server hostname DNS entry to a backup one with a different IP.

>> CIFS also doesn't make direct use of the TTL, and again this may be because it
>> uses the server address as part of the primary key for the superblock (see
>> cifs_match_super()).

When we try to reconnect after a failure (using (a) or just reconnecting
to same server) we resolve the host again to try to use any new IP (in
case (b) happened). This is done via upcalling using the request_key()
API.

The cifs.upcall prog (from cifs-utils) calls getaddrinfo() and sets a
key with a default TTL of 10mn [2][3] but if the system uses DNS caching
via nscd[1] there's no way to tell how long the old IP will remain in
use...

1: https://linux.die.net/man/8/nscd
2: https://github.com/piastry/cifs-utils/blob/9a8c21ad9e4510a83a3a41f7a04f763a4fe9ec09/cifs.upcall.c#L66
3: https://github.com/piastry/cifs-utils/blob/9a8c21ad9e4510a83a3a41f7a04f763a4fe9ec09/cifs.upcall.c#L783

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: What's a good default TTL for DNS keys in the kernel

[Steve French]

>> The question remains what the expected impact of TTL expiry is.  Will
>> the kernel just perform a new DNS query if it needs one?

For SMB3/CIFS mounts, Paulo added support last year for automatic
reconnect if the IP address of the server changes.  It also is helpful
when DFS (global name space) addresses change.

It does not require a remount for SMB3/CIFS

On Tue, Apr 14, 2020 at 11:09 AM David Howells <dhowells@redhat.com> wrote:
>
> Since key.dns_resolver isn't given a TTL for the address information obtained
> for getaddrinfo(), no expiry is set on dns_resolver keys in the kernel for
> NFS, CIFS or Ceph.  AFS gets one if it looks up a cell SRV or AFSDB record
> because that is looked up in the DNS directly, but it doesn't look up A or
> AAAA records, so doesn't get an expiry for the addresses themselves.
>
> I've previously asked the libc folks if there's a way to get this information
> exposed in struct addrinfo, but I don't think that ended up going anywhere -
> and, in any case, would take a few years to work through the system.
>
> For the moment, I think I should put a default on any dns_resolver keys and
> have it applied either by the kernel (configurable with a /proc/sys/ setting)
> or by the key.dnf_resolver program (configurable with an /etc file).
>
> Any suggestion as to the preferred default TTL?  10 minutes?
>
> David
>


-- 
Thanks,

Steve

Re: What's a good default TTL for DNS keys in the kernel

[Florian Weimer]

* Steve French:

>>> The question remains what the expected impact of TTL expiry is.  Will
>>> the kernel just perform a new DNS query if it needs one?
>
> For SMB3/CIFS mounts, Paulo added support last year for automatic
> reconnect if the IP address of the server changes.  It also is helpful
> when DFS (global name space) addresses change.

Do you have reference to the source code implementation?  Thanks.

Florian

Re: What's a good default TTL for DNS keys in the kernel

[Steve French]

commit 28eb24ff75c5ac130eb326b3b4d0dcecfc0f427d
Author: Paulo Alcantara <paulo@paulo.ac>
Date:   Tue Nov 20 15:16:36 2018 -0200

    cifs: Always resolve hostname before reconnecting

    In case a hostname resolves to a different IP address (e.g. long
    running mounts), make sure to resolve it every time prior to calling
    generic_ip_connect() in reconnect.
...

(Note that this patch may have some minor dependencies on a few other
DFS related patches that were merged immediately before it.
08744015492f cifs: Add support for failover in cifs_reconnect_tcon()
a3a53b760379 cifs: Add support for failover in smb2_reconnect()
23324407143d cifs: Only free DFS target list if we actually got one
e511d31753e3 cifs: start DFS cache refresher in cifs_mount()
93d5cb517db3 cifs: Add support for failover in cifs_reconnect()
4a367dc04435 cifs: Add support for failover in cifs_mount()
1c780228e9d4 cifs: Make use of DFS cache to get new DFS referrals

On Sat, Apr 18, 2020 at 1:11 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Steve French:
>
> >>> The question remains what the expected impact of TTL expiry is.  Will
> >>> the kernel just perform a new DNS query if it needs one?
> >
> > For SMB3/CIFS mounts, Paulo added support last year for automatic
> > reconnect if the IP address of the server changes.  It also is helpful
> > when DFS (global name space) addresses change.
>
> Do you have reference to the source code implementation?  Thanks.
>
> Florian
>


-- 
Thanks,

Steve

Re: What's a good default TTL for DNS keys in the kernel

[David Howells]

Steve French <smfrench@gmail.com> wrote:

> For SMB3/CIFS mounts, Paulo added support last year for automatic
> reconnect if the IP address of the server changes.  It also is helpful
> when DFS (global name space) addresses change.

What happens if the IP address the superblock is going to changes, then
another mount is made back to the original IP address?  Does the second mount
just pick the original superblock?

David

Re: What's a good default TTL for DNS keys in the kernel

[Paulo Alcantara]

David Howells <dhowells@redhat.com> writes:

> Steve French <smfrench@gmail.com> wrote:
>
>> For SMB3/CIFS mounts, Paulo added support last year for automatic
>> reconnect if the IP address of the server changes.  It also is helpful
>> when DFS (global name space) addresses change.
>
> What happens if the IP address the superblock is going to changes, then
> another mount is made back to the original IP address?  Does the second mount
> just pick the original superblock?

It is going to transparently reconnect to the new ip address, SMB share,
and cifs superblock is kept unchanged.  We, however, update internal
TCP_Server_Info structure to reflect new destination ip address.

For the second mount, since the hostname (extracted out of the UNC path
at mount time) resolves to a new ip address and that address was saved earlier
in TCP_Server_Info structure during reconnect, we will end up
reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().

Re: What's a good default TTL for DNS keys in the kernel

[David Howells]

Paulo Alcantara <pc@cjr.nz> wrote:

> >> For SMB3/CIFS mounts, Paulo added support last year for automatic
> >> reconnect if the IP address of the server changes.  It also is helpful
> >> when DFS (global name space) addresses change.
> >
> > What happens if the IP address the superblock is going to changes, then
> > another mount is made back to the original IP address?  Does the second mount
> > just pick the original superblock?
> 
> It is going to transparently reconnect to the new ip address, SMB share,
> and cifs superblock is kept unchanged.  We, however, update internal
> TCP_Server_Info structure to reflect new destination ip address.
> 
> For the second mount, since the hostname (extracted out of the UNC path
> at mount time) resolves to a new ip address and that address was saved earlier
> in TCP_Server_Info structure during reconnect, we will end up
> reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().

Would that be a bug?

David

Re: What's a good default TTL for DNS keys in the kernel

[Paulo Alcantara]

David Howells <dhowells@redhat.com> writes:

> Paulo Alcantara <pc@cjr.nz> wrote:
>
>> >> For SMB3/CIFS mounts, Paulo added support last year for automatic
>> >> reconnect if the IP address of the server changes.  It also is helpful
>> >> when DFS (global name space) addresses change.
>> >
>> > What happens if the IP address the superblock is going to changes, then
>> > another mount is made back to the original IP address?  Does the second mount
>> > just pick the original superblock?
>> 
>> It is going to transparently reconnect to the new ip address, SMB share,
>> and cifs superblock is kept unchanged.  We, however, update internal
>> TCP_Server_Info structure to reflect new destination ip address.
>> 
>> For the second mount, since the hostname (extracted out of the UNC path
>> at mount time) resolves to a new ip address and that address was saved earlier
>> in TCP_Server_Info structure during reconnect, we will end up
>> reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().
>
> Would that be a bug?

Probably.

I'm not sure how that code is supposed to work, TBH.

cifs - Race between IP address change and sget()?

[David Howells]

Paulo Alcantara <pc@cjr.nz> wrote:

> >> > What happens if the IP address the superblock is going to changes, then
> >> > another mount is made back to the original IP address?  Does the second
> >> > mount just pick the original superblock?
> >> 
> >> It is going to transparently reconnect to the new ip address, SMB share,
> >> and cifs superblock is kept unchanged.  We, however, update internal
> >> TCP_Server_Info structure to reflect new destination ip address.
> >> 
> >> For the second mount, since the hostname (extracted out of the UNC path
> >> at mount time) resolves to a new ip address and that address was saved
> >> earlier in TCP_Server_Info structure during reconnect, we will end up
> >> reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().
> >
> > Would that be a bug?
> 
> Probably.
> 
> I'm not sure how that code is supposed to work, TBH.

Hmmm...  I think there may be a race here then - but I'm not sure it can be
avoided or if it matters.

Since the address is part of the primary key to sget() for cifs, changing the
IP address will change the primary key.  Jeff tells me that this is governed
by a spinlock taken by cifs_match_super().  However, sget() may be busy
attaching a new mount to the old superblock under the sb_lock core vfs lock,
having already found a match.

Should the change of parameters made by cifs be effected with sb_lock held to
try and avoid ending up using the wrong superblock?

However, because the TCP_Server_Info is apparently updated, it looks like my
original concern is not actually a problem (the idea that if a mounted server
changes its IP address and then a new server comes online at the old IP
address, it might end up sharing superblocks because the IP address is part of
the key).

David

Re: cifs - Race between IP address change and sget()?

[Jeff Layton]

On Mon, 2020-04-20 at 23:14 +0100, David Howells wrote:
> Paulo Alcantara <pc@cjr.nz> wrote:
> 
> > > > > What happens if the IP address the superblock is going to changes, then
> > > > > another mount is made back to the original IP address?  Does the second
> > > > > mount just pick the original superblock?
> > > > 
> > > > It is going to transparently reconnect to the new ip address, SMB share,
> > > > and cifs superblock is kept unchanged.  We, however, update internal
> > > > TCP_Server_Info structure to reflect new destination ip address.
> > > > 
> > > > For the second mount, since the hostname (extracted out of the UNC path
> > > > at mount time) resolves to a new ip address and that address was saved
> > > > earlier in TCP_Server_Info structure during reconnect, we will end up
> > > > reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().
> > > 
> > > Would that be a bug?
> > 
> > Probably.
> > 
> > I'm not sure how that code is supposed to work, TBH.
> 
> Hmmm...  I think there may be a race here then - but I'm not sure it can be
> avoided or if it matters.
> 
> Since the address is part of the primary key to sget() for cifs, changing the
> IP address will change the primary key.  Jeff tells me that this is governed
> by a spinlock taken by cifs_match_super().  However, sget() may be busy
> attaching a new mount to the old superblock under the sb_lock core vfs lock,
> having already found a match.
> 

Not exactly. Both places that match TCP_Server_Info objects by address
hold the cifs_tcp_ses_lock. The address looks like it gets changed in
reconn_set_ipaddr, and the lock is not currently taken there, AFAICT. I
think it probably should be (at least around the cifs_convert_address
call).

> Should the change of parameters made by cifs be effected with sb_lock held to
> try and avoid ending up using the wrong superblock?
> 
> However, because the TCP_Server_Info is apparently updated, it looks like my
> original concern is not actually a problem (the idea that if a mounted server
> changes its IP address and then a new server comes online at the old IP
> address, it might end up sharing superblocks because the IP address is part of
> the key).
> 

I'm not sure we should concern ourselves with much more than just not
allowing addresses to change while matching/searching. If you're
standing up new servers at old addresses while you still have clients
are migrating, then you are probably Doing it Wrong.

-- 
Jeff Layton <jlayton@redhat.com>

Re: cifs - Race between IP address change and sget()?

[Ronnie Sahlberg]

----- Original Message -----
> From: "Jeff Layton" <jlayton@redhat.com>
> To: "David Howells" <dhowells@redhat.com>, "Paulo Alcantara" <pc@cjr.nz>
> Cc: viro@zeniv.linux.org.uk, "Steve French" <smfrench@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "CIFS"
> <linux-cifs@vger.kernel.org>, linux-afs@lists.infradead.org, ceph-devel@vger.kernel.org, keyrings@vger.kernel.org,
> "Network Development" <netdev@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, fweimer@redhat.com
> Sent: Tuesday, 21 April, 2020 8:30:37 AM
> Subject: Re: cifs - Race between IP address change and sget()?
> 
> On Mon, 2020-04-20 at 23:14 +0100, David Howells wrote:
> > Paulo Alcantara <pc@cjr.nz> wrote:
> > 
> > > > > > What happens if the IP address the superblock is going to changes,
> > > > > > then
> > > > > > another mount is made back to the original IP address?  Does the
> > > > > > second
> > > > > > mount just pick the original superblock?
> > > > > 
> > > > > It is going to transparently reconnect to the new ip address, SMB
> > > > > share,
> > > > > and cifs superblock is kept unchanged.  We, however, update internal
> > > > > TCP_Server_Info structure to reflect new destination ip address.
> > > > > 
> > > > > For the second mount, since the hostname (extracted out of the UNC
> > > > > path
> > > > > at mount time) resolves to a new ip address and that address was
> > > > > saved
> > > > > earlier in TCP_Server_Info structure during reconnect, we will end up
> > > > > reusing same cifs superblock as per
> > > > > fs/cifs/connect.c:cifs_match_super().
> > > > 
> > > > Would that be a bug?
> > > 
> > > Probably.
> > > 
> > > I'm not sure how that code is supposed to work, TBH.
> > 
> > Hmmm...  I think there may be a race here then - but I'm not sure it can be
> > avoided or if it matters.
> > 
> > Since the address is part of the primary key to sget() for cifs, changing
> > the
> > IP address will change the primary key.  Jeff tells me that this is
> > governed
> > by a spinlock taken by cifs_match_super().  However, sget() may be busy
> > attaching a new mount to the old superblock under the sb_lock core vfs
> > lock,
> > having already found a match.
> > 
> 
> Not exactly. Both places that match TCP_Server_Info objects by address
> hold the cifs_tcp_ses_lock. The address looks like it gets changed in
> reconn_set_ipaddr, and the lock is not currently taken there, AFAICT. I
> think it probably should be (at least around the cifs_convert_address
> call).

I think you are right. We need the spinlock around this call too.
I will send a patch to the list to add this.

> 
> > Should the change of parameters made by cifs be effected with sb_lock held
> > to
> > try and avoid ending up using the wrong superblock?
> > 
> > However, because the TCP_Server_Info is apparently updated, it looks like
> > my
> > original concern is not actually a problem (the idea that if a mounted
> > server
> > changes its IP address and then a new server comes online at the old IP
> > address, it might end up sharing superblocks because the IP address is part
> > of
> > the key).
> > 
> 
> I'm not sure we should concern ourselves with much more than just not
> allowing addresses to change while matching/searching. If you're
> standing up new servers at old addresses while you still have clients
> are migrating, then you are probably Doing it Wrong.

Agree. That is a migration process issue and not something we can/should
try to address in cifs.ko.



> 
> --
> Jeff Layton <jlayton@redhat.com>
> 
> 

Re: cifs - Race between IP address change and sget()?

[Steve French]

On Mon, Apr 20, 2020 at 8:30 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
>
>
>
>
> ----- Original Message -----
> > From: "Jeff Layton" <jlayton@redhat.com>
> > To: "David Howells" <dhowells@redhat.com>, "Paulo Alcantara" <pc@cjr.nz>
> > Cc: viro@zeniv.linux.org.uk, "Steve French" <smfrench@gmail.com>, "linux-nfs" <linux-nfs@vger.kernel.org>, "CIFS"
> > <linux-cifs@vger.kernel.org>, linux-afs@lists.infradead.org, ceph-devel@vger.kernel.org, keyrings@vger.kernel.org,
> > "Network Development" <netdev@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, fweimer@redhat.com
> > Sent: Tuesday, 21 April, 2020 8:30:37 AM
> > Subject: Re: cifs - Race between IP address change and sget()?
> >
> > On Mon, 2020-04-20 at 23:14 +0100, David Howells wrote:
> > > Paulo Alcantara <pc@cjr.nz> wrote:
> > >
> > > > > > > What happens if the IP address the superblock is going to changes,
> > > > > > > then
> > > > > > > another mount is made back to the original IP address?  Does the
> > > > > > > second
> > > > > > > mount just pick the original superblock?
> > > > > >
> > > > > > It is going to transparently reconnect to the new ip address, SMB
> > > > > > share,
> > > > > > and cifs superblock is kept unchanged.  We, however, update internal
> > > > > > TCP_Server_Info structure to reflect new destination ip address.
> > > > > >
> > > > > > For the second mount, since the hostname (extracted out of the UNC
> > > > > > path
> > > > > > at mount time) resolves to a new ip address and that address was
> > > > > > saved
> > > > > > earlier in TCP_Server_Info structure during reconnect, we will end up
> > > > > > reusing same cifs superblock as per
> > > > > > fs/cifs/connect.c:cifs_match_super().
> > > > >
> > > > > Would that be a bug?
> > > >
> > > > Probably.
> > > >
> > > > I'm not sure how that code is supposed to work, TBH.
> > >
> > > Hmmm...  I think there may be a race here then - but I'm not sure it can be
> > > avoided or if it matters.
> > >
> > > Since the address is part of the primary key to sget() for cifs, changing
> > > the
> > > IP address will change the primary key.  Jeff tells me that this is
> > > governed
> > > by a spinlock taken by cifs_match_super().  However, sget() may be busy
> > > attaching a new mount to the old superblock under the sb_lock core vfs
> > > lock,
> > > having already found a match.
> > >
> >
> > Not exactly. Both places that match TCP_Server_Info objects by address
> > hold the cifs_tcp_ses_lock. The address looks like it gets changed in
> > reconn_set_ipaddr, and the lock is not currently taken there, AFAICT. I
> > think it probably should be (at least around the cifs_convert_address
> > call).
>
> I think you are right. We need the spinlock around this call too.
> I will send a patch to the list to add this.
>
> >
> > > Should the change of parameters made by cifs be effected with sb_lock held
> > > to
> > > try and avoid ending up using the wrong superblock?
> > >
> > > However, because the TCP_Server_Info is apparently updated, it looks like
> > > my
> > > original concern is not actually a problem (the idea that if a mounted
> > > server
> > > changes its IP address and then a new server comes online at the old IP
> > > address, it might end up sharing superblocks because the IP address is part
> > > of
> > > the key).
> > >
> >
> > I'm not sure we should concern ourselves with much more than just not
> > allowing addresses to change while matching/searching. If you're
> > standing up new servers at old addresses while you still have clients
> > are migrating, then you are probably Doing it Wrong.
>
> Agree. That is a migration process issue and not something we can/should
> try to address in cifs.ko.

Yep

-- 
Thanks,

Steve

Re: cifs - Race between IP address change and sget()?

[Steve French]

On Mon, Apr 20, 2020 at 5:30 PM Jeff Layton <jlayton@redhat.com> wrote:
>
> On Mon, 2020-04-20 at 23:14 +0100, David Howells wrote:
> > Paulo Alcantara <pc@cjr.nz> wrote:
> >
> > > > > > What happens if the IP address the superblock is going to changes, then
> > > > > > another mount is made back to the original IP address?  Does the second
> > > > > > mount just pick the original superblock?
> > > > >
> > > > > It is going to transparently reconnect to the new ip address, SMB share,
> > > > > and cifs superblock is kept unchanged.  We, however, update internal
> > > > > TCP_Server_Info structure to reflect new destination ip address.
> > > > >
> > > > > For the second mount, since the hostname (extracted out of the UNC path
> > > > > at mount time) resolves to a new ip address and that address was saved
> > > > > earlier in TCP_Server_Info structure during reconnect, we will end up
> > > > > reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().
> > > >
> > > > Would that be a bug?
> > >
> > > Probably.
> > >
> > > I'm not sure how that code is supposed to work, TBH.
> >
> > Hmmm...  I think there may be a race here then - but I'm not sure it can be
> > avoided or if it matters.
> >
> > Since the address is part of the primary key to sget() for cifs, changing the
> > IP address will change the primary key.  Jeff tells me that this is governed
> > by a spinlock taken by cifs_match_super().  However, sget() may be busy
> > attaching a new mount to the old superblock under the sb_lock core vfs lock,
> > having already found a match.
> >
>
> Not exactly. Both places that match TCP_Server_Info objects by address
> hold the cifs_tcp_ses_lock. The address looks like it gets changed in
> reconn_set_ipaddr, and the lock is not currently taken there, AFAICT. I
> think it probably should be (at least around the cifs_convert_address
> call).
>
> > Should the change of parameters made by cifs be effected with sb_lock held to
> > try and avoid ending up using the wrong superblock?
> >
> > However, because the TCP_Server_Info is apparently updated, it looks like my
> > original concern is not actually a problem (the idea that if a mounted server
> > changes its IP address and then a new server comes online at the old IP
> > address, it might end up sharing superblocks because the IP address is part of
> > the key).
> >
>
> I'm not sure we should concern ourselves with much more than just not
> allowing addresses to change while matching/searching. If you're
> standing up new servers at old addresses while you still have clients
> are migrating, then you are probably Doing it Wrong.

Yes.   The most important thing is to support the reasonably
common scenario where the server's IP address changes (there are
fancier ways to handle this gracefully e.g. the "Witness Protocol" feature
of SMB3 mounts, but that is not always supported by servers and we
still need to add Witness protocol to clients - but current code allowing
SMB3 server IP address to change has helped some customers out)

-- 
Thanks,

Steve