| Message ID | 20170115181500.4465-1-leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, 2017-01-15 at 20:15 +0200, Leon Romanovsky wrote: > From: Jack Morgenstein <jackm@dev.mellanox.co.il> > > If IPV6 has not been enabled in the underlying kernel, we must avoid > calling IPV6 procedures in rdma_cm.ko. > > This requires using "IS_ENABLED(CONFIG_IPV6)" in "if" statements > surrounding any code which calls external IPV6 procedures. This seems strange.... > In the instance fixed here, procedure cma_bind_addr() called > ipv6_addr_type() -- which resulted in calling external procedure > __ipv6_addr_type(). > > Fixes: 6c26a77124ff ("RDMA/cma: fix IPv6 address resolution") > Cc: <stable@vger.kernel.org> # v4.2+ > Cc: Spencer Baugh <sbaugh@catern.com> > Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il> > Reviewed-by: Moni Shoua <monis@mellanox.com> > Signed-off-by: Leon Romanovsky <leon@kernel.org> > --- > drivers/infiniband/core/cma.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cma.c > b/drivers/infiniband/core/cma.c > index bd8d051..e19f19c 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -2822,7 +2822,8 @@ static int cma_bind_addr(struct rdma_cm_id *id, > struct sockaddr *src_addr, > if (!src_addr || !src_addr->sa_family) { > src_addr = (struct sockaddr *) &id- > >route.addr.src_addr; > src_addr->sa_family = dst_addr->sa_family; > - if (dst_addr->sa_family == AF_INET6) { Why this construct? Isn't the norm to simply surround the entire if statement with #if IS_ENABLED(CONFIG_IPV6) ... #endif > + if (IS_ENABLED(CONFIG_IPV6) && > + dst_addr->sa_family == AF_INET6) { > struct sockaddr_in6 *src_addr6 = (struct > sockaddr_in6 *) src_addr; > struct sockaddr_in6 *dst_addr6 = (struct > sockaddr_in6 *) dst_addr; > src_addr6->sin6_scope_id = dst_addr6- > >sin6_scope_id; > -- > 2.10.2 >
On Tue, Jan 24, 2017 at 04:28:50PM -0500, Doug Ledford wrote: > On Sun, 2017-01-15 at 20:15 +0200, Leon Romanovsky wrote: > > From: Jack Morgenstein <jackm@dev.mellanox.co.il> > > > > If IPV6 has not been enabled in the underlying kernel, we must avoid > > calling IPV6 procedures in rdma_cm.ko. > > > > This requires using "IS_ENABLED(CONFIG_IPV6)" in "if" statements > > surrounding any code which calls external IPV6 procedures. > > This seems strange.... > > > In the instance fixed here, procedure cma_bind_addr() called > > ipv6_addr_type() -- which resulted in calling external procedure > > __ipv6_addr_type(). > > > > Fixes: 6c26a77124ff ("RDMA/cma: fix IPv6 address resolution") > > Cc: <stable@vger.kernel.org> # v4.2+ > > Cc: Spencer Baugh <sbaugh@catern.com> > > Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il> > > Reviewed-by: Moni Shoua <monis@mellanox.com> > > Signed-off-by: Leon Romanovsky <leon@kernel.org> > > --- > > drivers/infiniband/core/cma.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/cma.c > > b/drivers/infiniband/core/cma.c > > index bd8d051..e19f19c 100644 > > --- a/drivers/infiniband/core/cma.c > > +++ b/drivers/infiniband/core/cma.c > > @@ -2822,7 +2822,8 @@ static int cma_bind_addr(struct rdma_cm_id *id, > > struct sockaddr *src_addr, > > if (!src_addr || !src_addr->sa_family) { > > src_addr = (struct sockaddr *) &id- > > >route.addr.src_addr; > > src_addr->sa_family = dst_addr->sa_family; > > - if (dst_addr->sa_family == AF_INET6) { > > Why this construct? Isn't the norm to simply surround the entire if > statement with > #if IS_ENABLED(CONFIG_IPV6) > ... > #endif It is common way to add dependency on specific config option directly to the flow if other "if" already exists. It gives clear view on the flow, eliminates the need to find corresponding #endif and put all constraints in one place. For example latest patches, bet there is a lot of code like this: commit 7ede8665f27cde7da69e8b2fbeaa1ed0664879c5 Author: Alexander Popov <alex.popov@linux.com> Date: Mon Dec 19 16:23:06 2016 -0800 arm64: setup: introduce kaslr_offset() commit b6f8a92c9ca835b4a079ecee8433d0d110398448 Author: Nicolas Pitre <nicolas.pitre@linaro.org> Date: Wed Dec 14 15:06:13 2016 -0800 posix-timers: give lazy compilers some help optimizing code away ..... > > > + if (IS_ENABLED(CONFIG_IPV6) && > > + dst_addr->sa_family == AF_INET6) { > > struct sockaddr_in6 *src_addr6 = (struct > > sockaddr_in6 *) src_addr; > > struct sockaddr_in6 *dst_addr6 = (struct > > sockaddr_in6 *) dst_addr; > > src_addr6->sin6_scope_id = dst_addr6- > > >sin6_scope_id; > > -- > > 2.10.2 > > > -- > Doug Ledford <dledford@redhat.com> > GPG KeyID: B826A3330E572FDD > > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
On 1/25/2017 1:51 AM, Leon Romanovsky wrote: > On Tue, Jan 24, 2017 at 04:28:50PM -0500, Doug Ledford wrote: >> On Sun, 2017-01-15 at 20:15 +0200, Leon Romanovsky wrote: >>> From: Jack Morgenstein <jackm@dev.mellanox.co.il> >>> >>> If IPV6 has not been enabled in the underlying kernel, we must avoid >>> calling IPV6 procedures in rdma_cm.ko. >>> >>> This requires using "IS_ENABLED(CONFIG_IPV6)" in "if" statements >>> surrounding any code which calls external IPV6 procedures. >> >> This seems strange.... >> >>> In the instance fixed here, procedure cma_bind_addr() called >>> ipv6_addr_type() -- which resulted in calling external procedure >>> __ipv6_addr_type(). >>> >>> Fixes: 6c26a77124ff ("RDMA/cma: fix IPv6 address resolution") >>> Cc: <stable@vger.kernel.org> # v4.2+ >>> Cc: Spencer Baugh <sbaugh@catern.com> >>> Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il> >>> Reviewed-by: Moni Shoua <monis@mellanox.com> >>> Signed-off-by: Leon Romanovsky <leon@kernel.org> >>> --- >>> drivers/infiniband/core/cma.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/infiniband/core/cma.c >>> b/drivers/infiniband/core/cma.c >>> index bd8d051..e19f19c 100644 >>> --- a/drivers/infiniband/core/cma.c >>> +++ b/drivers/infiniband/core/cma.c >>> @@ -2822,7 +2822,8 @@ static int cma_bind_addr(struct rdma_cm_id *id, >>> struct sockaddr *src_addr, >>> if (!src_addr || !src_addr->sa_family) { >>> src_addr = (struct sockaddr *) &id- >>>> route.addr.src_addr; >>> src_addr->sa_family = dst_addr->sa_family; >>> - if (dst_addr->sa_family == AF_INET6) { >> >> Why this construct? Isn't the norm to simply surround the entire if >> statement with >> #if IS_ENABLED(CONFIG_IPV6) >> ... >> #endif > > It is common way to add dependency on specific config option directly to > the flow if other "if" already exists. It gives clear view on the flow, > eliminates the need to find corresponding #endif and put all constraints > in one place. OK, that makes sense, I just hadn't seen it that way. I'll add this to the -rc queue.
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index bd8d051..e19f19c 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -2822,7 +2822,8 @@ static int cma_bind_addr(struct rdma_cm_id *id, struct sockaddr *src_addr, if (!src_addr || !src_addr->sa_family) { src_addr = (struct sockaddr *) &id->route.addr.src_addr; src_addr->sa_family = dst_addr->sa_family; - if (dst_addr->sa_family == AF_INET6) { + if (IS_ENABLED(CONFIG_IPV6) && + dst_addr->sa_family == AF_INET6) { struct sockaddr_in6 *src_addr6 = (struct sockaddr_in6 *) src_addr; struct sockaddr_in6 *dst_addr6 = (struct sockaddr_in6 *) dst_addr; src_addr6->sin6_scope_id = dst_addr6->sin6_scope_id;