On Mon, Dec 20, 2021 at 3:55 AM Enzo Matsumiya <ematsumiya@xxxxxxx> 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@xxxxxxxxxxxxx> > > 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@xxxxxxxxxx> > > Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx> > > --- > > 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
