| Message ID | 4D9431B3.2070305@parallels.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Rob Landley (rlandley@parallels.com): > From: Rob Landley <rlandley@parallels.com> > > The auth_unix cache needs to check network namespace when comparing > addresses. Add field to struct ip_map and extra argument to > __ip_map_lookup() so it has the information to do so, and add test. > > Signed-off-by: Rob Landley <rlandley@parallels.com> > --- > > net/sunrpc/svcauth_unix.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c > index 30916b0..63a2fa7 100644 > --- a/net/sunrpc/svcauth_unix.c > +++ b/net/sunrpc/svcauth_unix.c > @@ -14,6 +14,7 @@ > #include <net/sock.h> > #include <net/ipv6.h> > #include <linux/kernel.h> > +#include <linux/user_namespace.h> > #define RPCDBG_FACILITY RPCDBG_AUTH > > #include <linux/sunrpc/clnt.h> > @@ -94,6 +95,7 @@ struct ip_map { > struct cache_head h; > char m_class[8]; /* e.g. "nfsd" */ > struct in6_addr m_addr; > + struct net *m_net; > struct unix_domain *m_client; > #ifdef CONFIG_NFSD_DEPRECATED > int m_add_change; > @@ -134,6 +136,7 @@ static int ip_map_match(struct cache_head *corig, struct cache_head *cnew) > struct ip_map *orig = container_of(corig, struct ip_map, h); > struct ip_map *new = container_of(cnew, struct ip_map, h); > return strcmp(orig->m_class, new->m_class) == 0 && > + net_eq(orig->m_net, new->m_net) && > ipv6_addr_equal(&orig->m_addr, &new->m_addr); > } > static void ip_map_init(struct cache_head *cnew, struct cache_head *citem) > @@ -142,6 +145,7 @@ static void ip_map_init(struct cache_head *cnew, struct cache_head *citem) > struct ip_map *item = container_of(citem, struct ip_map, h); > > strcpy(new->m_class, item->m_class); > + new->m_net = item->m_net; Does this need to take a reference? Or is there no way for an entry to outlive its netns? It sort of looks like svcauth_unix_info_release will ensure that doesn't happen, but I'm not convinced because other parts of the kernel can get to ip_map_init through the struct cache_detail. > ipv6_addr_copy(&new->m_addr, &item->m_addr); > } > static void update(struct cache_head *cnew, struct cache_head *citem) > @@ -186,7 +190,7 @@ static int ip_map_upcall(struct cache_detail *cd, struct cache_head *h) > return sunrpc_cache_pipe_upcall(cd, h, ip_map_request); > } > > -static struct ip_map *__ip_map_lookup(struct cache_detail *cd, char *class, struct in6_addr *addr); > +static struct ip_map *__ip_map_lookup(struct cache_detail *cd, struct net *net, char *class, struct in6_addr *addr); > static int __ip_map_update(struct cache_detail *cd, struct ip_map *ipm, struct unix_domain *udom, time_t expiry); > > static int ip_map_parse(struct cache_detail *cd, > @@ -256,7 +260,8 @@ static int ip_map_parse(struct cache_detail *cd, > dom = NULL; > > /* IPv6 scope IDs are ignored for now */ > - ipmp = __ip_map_lookup(cd, class, &sin6.sin6_addr); > + ipmp = __ip_map_lookup(cd, current->nsproxy->net_ns, class, > + &sin6.sin6_addr); > if (ipmp) { > err = __ip_map_update(cd, ipmp, > container_of(dom, struct unix_domain, h), > @@ -301,13 +306,14 @@ static int ip_map_show(struct seq_file *m, > } > > > -static struct ip_map *__ip_map_lookup(struct cache_detail *cd, char *class, > - struct in6_addr *addr) > +static struct ip_map *__ip_map_lookup(struct cache_detail *cd, struct net *net, > + char *class, struct in6_addr *addr) > { > struct ip_map ip; > struct cache_head *ch; > > strcpy(ip.m_class, class); > + ip.m_net = net; > ipv6_addr_copy(&ip.m_addr, addr); > ch = sunrpc_cache_lookup(cd, &ip.h, > hash_str(class, IP_HASHBITS) ^ > @@ -325,7 +331,7 @@ static inline struct ip_map *ip_map_lookup(struct net *net, char *class, > struct sunrpc_net *sn; > > sn = net_generic(net, sunrpc_net_id); > - return __ip_map_lookup(sn->ip_map_cache, class, addr); > + return __ip_map_lookup(sn->ip_map_cache, net, class, addr); > } > > static int __ip_map_update(struct cache_detail *cd, struct ip_map *ipm, > @@ -748,8 +754,9 @@ svcauth_unix_set_client(struct svc_rqst *rqstp) > > ipm = ip_map_cached_get(xprt); > if (ipm == NULL) > - ipm = __ip_map_lookup(sn->ip_map_cache, rqstp->rq_server->sv_program->pg_class, > - &sin6->sin6_addr); > + ipm = __ip_map_lookup(sn->ip_map_cache, net, > + rqstp->rq_server->sv_program->pg_class, > + &sin6->sin6_addr); > > if (ipm == NULL) > return SVC_DENIED; > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/04/2011 10:46 PM, Serge E. Hallyn wrote: > Quoting Rob Landley (rlandley@parallels.com): >> From: Rob Landley <rlandley@parallels.com> >> >> The auth_unix cache needs to check network namespace when comparing >> addresses. Add field to struct ip_map and extra argument to >> __ip_map_lookup() so it has the information to do so, and add test. >> >> Signed-off-by: Rob Landley <rlandley@parallels.com> >> --- >> >> net/sunrpc/svcauth_unix.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c >> index 30916b0..63a2fa7 100644 >> --- a/net/sunrpc/svcauth_unix.c >> +++ b/net/sunrpc/svcauth_unix.c code code code >> @@ -142,6 +145,7 @@ static void ip_map_init(struct cache_head *cnew, struct cache_head *citem) >> struct ip_map *item = container_of(citem, struct ip_map, h); >> >> strcpy(new->m_class, item->m_class); >> + new->m_net = item->m_net; > > Does this need to take a reference? Or is there no way for an > entry to outlive its netns? It sort of looks like > svcauth_unix_info_release will ensure that doesn't happen, but > I'm not convinced because other parts of the kernel can get > to ip_map_init through the struct cache_detail. When I wrote this I thought the transport's get_net() and put_net() would pin it, but after re-reading, the sunrpc code is disgustingly convoluted enough that I can't easily reconstruct my earlier reasoning. I'll add a get_net() and put_net() just to not have to worry about it. Thanks, Rob -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/08/2011 10:08 AM, Rob Landley wrote: > On 04/04/2011 10:46 PM, Serge E. Hallyn wrote: >> Does this need to take a reference? Or is there no way for an >> entry to outlive its netns? It sort of looks like >> svcauth_unix_info_release will ensure that doesn't happen, but >> I'm not convinced because other parts of the kernel can get >> to ip_map_init through the struct cache_detail. > > When I wrote this I thought the transport's get_net() and put_net() > would pin it, but after re-reading, the sunrpc code is disgustingly > convoluted enough that I can't easily reconstruct my earlier reasoning. > I'll add a get_net() and put_net() just to not have to worry about it. Ah-ha! Stanislav Kinsbursky helped me reconstruct some of the reasoning: we don't need to take a reference because we never actually dereference the struct net *, all we do is feed them to net_eq() which just compares the pointers for equality. (The inline function exists so it can compile to a constant "return 1" when configured out.) So if the network context did go away (which still shouldn't happen between the rpc_xprt and the struct nfs_client having references to it) we still wouldn't have a use-after-free problem because we're not looking at the memory, just the pointer. So I shouldn't need to add get_net() and put_net() to the cache. Sound about right? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Rob Landley (rlandley@parallels.com): > On 04/08/2011 10:08 AM, Rob Landley wrote: > > On 04/04/2011 10:46 PM, Serge E. Hallyn wrote: > >> Does this need to take a reference? Or is there no way for an > >> entry to outlive its netns? It sort of looks like > >> svcauth_unix_info_release will ensure that doesn't happen, but > >> I'm not convinced because other parts of the kernel can get > >> to ip_map_init through the struct cache_detail. > > > > When I wrote this I thought the transport's get_net() and put_net() > > would pin it, but after re-reading, the sunrpc code is disgustingly > > convoluted enough that I can't easily reconstruct my earlier reasoning. > > I'll add a get_net() and put_net() just to not have to worry about it. > > Ah-ha! > > Stanislav Kinsbursky helped me reconstruct some of the reasoning: we > don't need to take a reference because we never actually dereference the > struct net *, all we do is feed them to net_eq() which just compares the > pointers for equality. (The inline function exists so it can compile to > a constant "return 1" when configured out.) > > So if the network context did go away (which still shouldn't happen > between the rpc_xprt and the struct nfs_client having references to it) > we still wouldn't have a use-after-free problem because we're not > looking at the memory, just the pointer. Besides use-after-free, the other concern is an invalid net_eq() result due to the * being re-used for a new netns. I assume that's deemed "super-duper impossible" again bc of the rpc_xprt/nfs_client references to it? > So I shouldn't need to add get_net() and put_net() to the cache. Sound > about right? thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 11, 2011 at 4:36 PM, Serge E. Hallyn <serge@hallyn.com> wrote: > Besides use-after-free, the other concern is an invalid net_eq() > result due to the * being re-used for a new netns. Exactly. "struct net" dies last, no exceptions, anything different is a disaster. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/11/2011 08:57 AM, Alexey Dobriyan wrote: > On Mon, Apr 11, 2011 at 4:36 PM, Serge E. Hallyn <serge@hallyn.com> wrote: > >> Besides use-after-free, the other concern is an invalid net_eq() >> result due to the * being re-used for a new netns. > > Exactly. > > "struct net" dies last, no exceptions, anything different is a disaster. Actually the patch turns out to be irrelevant because Pavel made the whole thing into a pernet subsystem, so it was already fixed in a different way. (Commit 2f72c9b7. My bad, I initially started working against an NFS tree with an older kernel version, this fix was to a different source file so my patch still applied, and I just regression tested that it worked, not that it still failed without it. Just caught it now.) I actually did talk about addressing potential reuse of the net * in my email with Stanislav (point of the patch was to allow one cache to maintain two different contexts at the same time; a situation that changed the meaning of an existing cache entry by requiring the old context to go away didn't introduce any new problems that a single context didn't already have due to DNAT, servers moving, administrators changing credentials on the server). But sort of a moot point now. I believe the third patch can be dropped entirely. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c index 30916b0..63a2fa7 100644 --- a/net/sunrpc/svcauth_unix.c +++ b/net/sunrpc/svcauth_unix.c @@ -14,6 +14,7 @@ #include <net/sock.h> #include <net/ipv6.h> #include <linux/kernel.h> +#include <linux/user_namespace.h> #define RPCDBG_FACILITY RPCDBG_AUTH #include <linux/sunrpc/clnt.h> @@ -94,6 +95,7 @@ struct ip_map { struct cache_head h; char m_class[8]; /* e.g. "nfsd" */ struct in6_addr m_addr; + struct net *m_net; struct unix_domain *m_client; #ifdef CONFIG_NFSD_DEPRECATED int m_add_change; @@ -134,6 +136,7 @@ static int ip_map_match(struct cache_head *corig, struct cache_head *cnew) struct ip_map *orig = container_of(corig, struct ip_map, h); struct ip_map *new = container_of(cnew, struct ip_map, h); return strcmp(orig->m_class, new->m_class) == 0 && + net_eq(orig->m_net, new->m_net) && ipv6_addr_equal(&orig->m_addr, &new->m_addr); } static void ip_map_init(struct cache_head *cnew, struct cache_head *citem) @@ -142,6 +145,7 @@ static void ip_map_init(struct cache_head *cnew, struct cache_head *citem) struct ip_map *item = container_of(citem, struct ip_map, h); strcpy(new->m_class, item->m_class); + new->m_net = item->m_net; ipv6_addr_copy(&new->m_addr, &item->m_addr); } static void update(struct cache_head *cnew, struct cache_head *citem) @@ -186,7 +190,7 @@ static int ip_map_upcall(struct cache_detail *cd, struct cache_head *h) return sunrpc_cache_pipe_upcall(cd, h, ip_map_request); } -static struct ip_map *__ip_map_lookup(struct cache_detail *cd, char *class, struct in6_addr *addr); +static struct ip_map *__ip_map_lookup(struct cache_detail *cd, struct net *net, char *class, struct in6_addr *addr); static int __ip_map_update(struct cache_detail *cd, struct ip_map *ipm, struct unix_domain *udom, time_t expiry); static int ip_map_parse(struct cache_detail *cd, @@ -256,7 +260,8 @@ static int ip_map_parse(struct cache_detail *cd, dom = NULL; /* IPv6 scope IDs are ignored for now */ - ipmp = __ip_map_lookup(cd, class, &sin6.sin6_addr); + ipmp = __ip_map_lookup(cd, current->nsproxy->net_ns, class, + &sin6.sin6_addr); if (ipmp) { err = __ip_map_update(cd, ipmp, container_of(dom, struct unix_domain, h), @@ -301,13 +306,14 @@ static int ip_map_show(struct seq_file *m, } -static struct ip_map *__ip_map_lookup(struct cache_detail *cd, char *class, - struct in6_addr *addr) +static struct ip_map *__ip_map_lookup(struct cache_detail *cd, struct net *net, + char *class, struct in6_addr *addr) { struct ip_map ip; struct cache_head *ch; strcpy(ip.m_class, class); + ip.m_net = net; ipv6_addr_copy(&ip.m_addr, addr); ch = sunrpc_cache_lookup(cd, &ip.h, hash_str(class, IP_HASHBITS) ^ @@ -325,7 +331,7 @@ static inline struct ip_map *ip_map_lookup(struct net *net, char *class, struct sunrpc_net *sn; sn = net_generic(net, sunrpc_net_id); - return __ip_map_lookup(sn->ip_map_cache, class, addr); + return __ip_map_lookup(sn->ip_map_cache, net, class, addr); } static int __ip_map_update(struct cache_detail *cd, struct ip_map *ipm, @@ -748,8 +754,9 @@ svcauth_unix_set_client(struct svc_rqst *rqstp) ipm = ip_map_cached_get(xprt); if (ipm == NULL) - ipm = __ip_map_lookup(sn->ip_map_cache, rqstp->rq_server->sv_program->pg_class, - &sin6->sin6_addr); + ipm = __ip_map_lookup(sn->ip_map_cache, net, + rqstp->rq_server->sv_program->pg_class, + &sin6->sin6_addr); if (ipm == NULL) return SVC_DENIED;