[PATCH] cifs: invalidate dns resolver keys after use

[PATCH] cifs: invalidate dns resolver keys after use

[Shyam Prasad N]

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

Hi Steve/Paulo/David,

Please review the attached patch.

I noticed that DNS resolution did not always upcall to userspace when
the IP address changed. This addresses the fix for it.

I would even recommend CC:stable for this one.

-- 
Regards,
Shyam

[-- Attachment #2: 0001-cifs-invalidate-dns-resolver-keys-after-use.patch --]
[-- Type: application/octet-stream, Size: 1620 bytes --]

From 604ab4c350c2552daa8e77f861a54032b49bc706 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Sat, 18 Dec 2021 17:28:10 +0000
Subject: [PATCH] cifs: invalidate dns resolver keys after use

We rely on dns resolver module to upcall to userspace
using request_key and get us the DNS mapping.
However, the invalidate arg for dns_query was set
to false, which meant that the key created during the
first call for a hostname would continue to be cached
till it expires. This expiration period depends on
how the dns_resolver is configured.

Fixing this by setting invalidate=true during dns_query.
This means that the key will be cleaned up by dns_resolver
soon after it returns the data. This also means that
the dns_resolver subsystem will not cache the key for
an interval indicated by the DNS records TTL. But this is
okay since we use the TTL value returned to schedule the
next lookup.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/dns_resolve.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
index 0458d28d71aa..8890af1537ef 100644
--- a/fs/cifs/dns_resolve.c
+++ b/fs/cifs/dns_resolve.c
@@ -66,7 +66,7 @@ dns_resolve_server_name_to_ip(const char *unc, char **ip_addr, time64_t *expiry)
 
 	/* Perform the upcall */
 	rc = dns_query(current->nsproxy->net_ns, NULL, hostname, len,
-		       NULL, ip_addr, expiry, false);
+		       NULL, ip_addr, expiry, true);
 	if (rc < 0)
 		cifs_dbg(FYI, "%s: unable to resolve: %*.*s\n",
 			 __func__, len, len, hostname);

Re: [PATCH] cifs: invalidate dns resolver keys after use

[Enzo Matsumiya]

Hi Shyam,

On 12/18, Shyam Prasad N wrote:
>Hi Steve/Paulo/David,
>
>Please review the attached patch.
>
>I noticed that DNS resolution did not always upcall to userspace when
>the IP address changed. This addresses the fix for it.
>
>I would even recommend CC:stable for this one.

(I'm pasting the patch here so I can comment inline)

> From 604ab4c350c2552daa8e77f861a54032b49bc706 Mon Sep 17 00:00:00 2001
> From: Shyam Prasad N <sprasad@microsoft.com>
> Date: Sat, 18 Dec 2021 17:28:10 +0000
> Subject: [PATCH] cifs: invalidate dns resolver keys after use
> 
> We rely on dns resolver module to upcall to userspace
> using request_key and get us the DNS mapping.
> However, the invalidate arg for dns_query was set
> to false, which meant that the key created during the
> first call for a hostname would continue to be cached
> till it expires. This expiration period depends on
> how the dns_resolver is configured.

Ok.

> 
> Fixing this by setting invalidate=true during dns_query.
> This means that the key will be cleaned up by dns_resolver
> soon after it returns the data. This also means that
> the dns_resolver subsystem will not cache the key for
> an interval indicated by the DNS records TTL.

This is ok too, which is an approach that I did try before, but
didn't work (see below).

> But this is
> okay since we use the TTL value returned to schedule the
> next lookup.

This is an incorrect assumption. keyutils' key.dns_resolve uses
getaddrinfo() for A/AAAA queries, which doesn't contain DNS TTL
information.

Meaning that the TTL returned to dns_resolve_server_name_to_ip() is
actually either key.dns_resolve.conf's default_ttl value, or the default
key_expiry value (5).

I have a patch ready (working, but still testing) for keyutils that implements
a "common" DNS interface, and the caller only specifies the query type,
which is then resolved via res_nquery(). This way, all returned records
have generic their metadata (TTL included), along with type-specific
metadata (e.g. AFSDB or SRV specifics) as well.

Another option/suggestion would be to:
1. decrease SMB_DNS_RESOLVE_INTERVAL_DEFAULT
2. and/or make it user-configurable via sysfs
3. call dns_query() with expiry=NULL and invalidate=true

So we'd use keyutils exclusively for kernel-userspace communication, and
handle the expiration checking/configuration on cifs side.

> 
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/cifs/dns_resolve.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
> index 0458d28d71aa..8890af1537ef 100644
> --- a/fs/cifs/dns_resolve.c
> +++ b/fs/cifs/dns_resolve.c
> @@ -66,7 +66,7 @@ dns_resolve_server_name_to_ip(const char *unc, char **ip_addr, time64_t *expiry)
>  
>  	/* Perform the upcall */
>  	rc = dns_query(current->nsproxy->net_ns, NULL, hostname, len,
> -		       NULL, ip_addr, expiry, false);
> +		       NULL, ip_addr, expiry, true);
>  	if (rc < 0)
>  		cifs_dbg(FYI, "%s: unable to resolve: %*.*s\n",
>  			 __func__, len, len, hostname);


Cheers,

Enzo

Re: [PATCH] cifs: invalidate dns resolver keys after use

[David Howells]

Shyam Prasad N <nspmangalore@gmail.com> wrote:

> From 604ab4c350c2552daa8e77f861a54032b49bc706 Mon Sep 17 00:00:00 2001
> From: Shyam Prasad N <sprasad@microsoft.com>
> Date: Sat, 18 Dec 2021 17:28:10 +0000
> Subject: [PATCH] cifs: invalidate dns resolver keys after use
> 
> We rely on dns resolver module to upcall to userspace
> using request_key and get us the DNS mapping.
> However, the invalidate arg for dns_query was set
> to false, which meant that the key created during the
> first call for a hostname would continue to be cached
> till it expires. This expiration period depends on
> how the dns_resolver is configured.
> 
> Fixing this by setting invalidate=true during dns_query.
> This means that the key will be cleaned up by dns_resolver
> soon after it returns the data. This also means that
> the dns_resolver subsystem will not cache the key for
> an interval indicated by the DNS records TTL. But this is
> okay since we use the TTL value returned to schedule the
> next lookup.
> 
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH] cifs: invalidate dns resolver keys after use

[Shyam Prasad N]

On Mon, Dec 20, 2021 at 3:55 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> Hi Shyam,
>
> On 12/18, Shyam Prasad N wrote:
> >Hi Steve/Paulo/David,
> >
> >Please review the attached patch.
> >
> >I noticed that DNS resolution did not always upcall to userspace when
> >the IP address changed. This addresses the fix for it.
> >
> >I would even recommend CC:stable for this one.
>
> (I'm pasting the patch here so I can comment inline)
>
> > From 604ab4c350c2552daa8e77f861a54032b49bc706 Mon Sep 17 00:00:00 2001
> > From: Shyam Prasad N <sprasad@microsoft.com>
> > Date: Sat, 18 Dec 2021 17:28:10 +0000
> > Subject: [PATCH] cifs: invalidate dns resolver keys after use
> >
> > We rely on dns resolver module to upcall to userspace
> > using request_key and get us the DNS mapping.
> > However, the invalidate arg for dns_query was set
> > to false, which meant that the key created during the
> > first call for a hostname would continue to be cached
> > till it expires. This expiration period depends on
> > how the dns_resolver is configured.
>
> Ok.
>
> >
> > Fixing this by setting invalidate=true during dns_query.
> > This means that the key will be cleaned up by dns_resolver
> > soon after it returns the data. This also means that
> > the dns_resolver subsystem will not cache the key for
> > an interval indicated by the DNS records TTL.
>
> This is ok too, which is an approach that I did try before, but
> didn't work (see below).
>
> > But this is
> > okay since we use the TTL value returned to schedule the
> > next lookup.
>
> This is an incorrect assumption. keyutils' key.dns_resolve uses
> getaddrinfo() for A/AAAA queries, which doesn't contain DNS TTL
> information.
>
> Meaning that the TTL returned to dns_resolve_server_name_to_ip() is
> actually either key.dns_resolve.conf's default_ttl value, or the default
> key_expiry value (5).
>
> I have a patch ready (working, but still testing) for keyutils that implements
> a "common" DNS interface, and the caller only specifies the query type,
> which is then resolved via res_nquery(). This way, all returned records
> have generic their metadata (TTL included), along with type-specific
> metadata (e.g. AFSDB or SRV specifics) as well.
>

Hi Enzo,
This would actually be very useful, if you can get it to work.

> Another option/suggestion would be to:
> 1. decrease SMB_DNS_RESOLVE_INTERVAL_DEFAULT
> 2. and/or make it user-configurable via sysfs
> 3. call dns_query() with expiry=NULL and invalidate=true
>
> So we'd use keyutils exclusively for kernel-userspace communication, and
> handle the expiration checking/configuration on cifs side.

Having such an option is useful. Although, getting the right TTL is
important. Especially for Azure workloads.

>
> >
> > Cc: David Howells <dhowells@redhat.com>
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/cifs/dns_resolve.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
> > index 0458d28d71aa..8890af1537ef 100644
> > --- a/fs/cifs/dns_resolve.c
> > +++ b/fs/cifs/dns_resolve.c
> > @@ -66,7 +66,7 @@ dns_resolve_server_name_to_ip(const char *unc, char **ip_addr, time64_t *expiry)
> >
> >       /* Perform the upcall */
> >       rc = dns_query(current->nsproxy->net_ns, NULL, hostname, len,
> > -                    NULL, ip_addr, expiry, false);
> > +                    NULL, ip_addr, expiry, true);
> >       if (rc < 0)
> >               cifs_dbg(FYI, "%s: unable to resolve: %*.*s\n",
> >                        __func__, len, len, hostname);
>
>
> Cheers,
>
> Enzo



-- 
Regards,
Shyam

Re: [PATCH] cifs: invalidate dns resolver keys after use

[David Howells]

Shyam Prasad N <nspmangalore@gmail.com> wrote:

> Having such an option is useful. Although, getting the right TTL is
> important.

That is the real trick as the libc interface you're supposed to use these days
doesn't give you that information - and, indeed, might draw from a source that
doesn't have such information.

David

Re: [PATCH] cifs: invalidate dns resolver keys after use

[Enzo Matsumiya]

On 12/23, David Howells wrote:
>Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
>> Having such an option is useful. Although, getting the right TTL is
>> important.
>
>That is the real trick as the libc interface you're supposed to use these days
>doesn't give you that information - and, indeed, might draw from a source that
>doesn't have such information.

I'm not sure I understand. I'm using res_nquery() on my to-be-proposed
patch and it works fine.

Re: [PATCH] cifs: invalidate dns resolver keys after use

[David Howells]

Enzo Matsumiya <ematsumiya@suse.de> wrote:

> I'm not sure I understand. I'm using res_nquery() on my to-be-proposed
> patch and it works fine.

You're supposed to use getaddrinfo() these days, apparently.  The info you're
looking for might not be in the DNS.

David