* [PATCH] cifs: invalidate dns resolver keys after use @ 2021-12-18 17:53 Shyam Prasad N 2021-12-19 22:25 ` Enzo Matsumiya 2021-12-20 12:55 ` David Howells 0 siblings, 2 replies; 7+ messages in thread From: Shyam Prasad N @ 2021-12-18 17:53 UTC (permalink / raw) To: Steve French, Paulo Alcantara, David Howells, CIFS [-- 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); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: invalidate dns resolver keys after use 2021-12-18 17:53 [PATCH] cifs: invalidate dns resolver keys after use Shyam Prasad N @ 2021-12-19 22:25 ` Enzo Matsumiya 2021-12-23 5:02 ` Shyam Prasad N 2021-12-23 8:42 ` David Howells 2021-12-20 12:55 ` David Howells 1 sibling, 2 replies; 7+ messages in thread From: Enzo Matsumiya @ 2021-12-19 22:25 UTC (permalink / raw) To: Shyam Prasad N; +Cc: Steve French, Paulo Alcantara, David Howells, CIFS 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: invalidate dns resolver keys after use 2021-12-19 22:25 ` Enzo Matsumiya @ 2021-12-23 5:02 ` Shyam Prasad N 2021-12-23 8:42 ` David Howells 1 sibling, 0 replies; 7+ messages in thread From: Shyam Prasad N @ 2021-12-23 5:02 UTC (permalink / raw) To: Enzo Matsumiya; +Cc: Steve French, Paulo Alcantara, David Howells, CIFS 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: invalidate dns resolver keys after use 2021-12-19 22:25 ` Enzo Matsumiya 2021-12-23 5:02 ` Shyam Prasad N @ 2021-12-23 8:42 ` David Howells 2021-12-23 20:33 ` Enzo Matsumiya 2022-01-05 13:12 ` David Howells 1 sibling, 2 replies; 7+ messages in thread From: David Howells @ 2021-12-23 8:42 UTC (permalink / raw) To: Shyam Prasad N Cc: dhowells, Enzo Matsumiya, Steve French, Paulo Alcantara, CIFS 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: invalidate dns resolver keys after use 2021-12-23 8:42 ` David Howells @ 2021-12-23 20:33 ` Enzo Matsumiya 2022-01-05 13:12 ` David Howells 1 sibling, 0 replies; 7+ messages in thread From: Enzo Matsumiya @ 2021-12-23 20:33 UTC (permalink / raw) To: David Howells; +Cc: Shyam Prasad N, Steve French, Paulo Alcantara, CIFS 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: invalidate dns resolver keys after use 2021-12-23 8:42 ` David Howells 2021-12-23 20:33 ` Enzo Matsumiya @ 2022-01-05 13:12 ` David Howells 1 sibling, 0 replies; 7+ messages in thread From: David Howells @ 2022-01-05 13:12 UTC (permalink / raw) To: Enzo Matsumiya Cc: dhowells, Shyam Prasad N, Steve French, Paulo Alcantara, CIFS 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cifs: invalidate dns resolver keys after use 2021-12-18 17:53 [PATCH] cifs: invalidate dns resolver keys after use Shyam Prasad N 2021-12-19 22:25 ` Enzo Matsumiya @ 2021-12-20 12:55 ` David Howells 1 sibling, 0 replies; 7+ messages in thread From: David Howells @ 2021-12-20 12:55 UTC (permalink / raw) To: Shyam Prasad N; +Cc: dhowells, Steve French, Paulo Alcantara, CIFS 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-05 13:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-18 17:53 [PATCH] cifs: invalidate dns resolver keys after use Shyam Prasad N 2021-12-19 22:25 ` Enzo Matsumiya 2021-12-23 5:02 ` Shyam Prasad N 2021-12-23 8:42 ` David Howells 2021-12-23 20:33 ` Enzo Matsumiya 2022-01-05 13:12 ` David Howells 2021-12-20 12:55 ` David Howells
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.