| Message ID | 531DCB7D.9020104@acm.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hi, Le lundi 10 mars 2014 à 15:26 +0100, Bart Van Assche a écrit : > On 03/10/14 14:33, Yann Droneaud wrote: > > Le lundi 10 mars 2014 à 13:22 +0100, Bart Van Assche a écrit : > >> Suppress the following sparse warning: > >> include/rdma/ib_addr.h:187:24: warning: cast removes address space of expression > > > > You should explain why there's a warning here, and why is it safe to > > cast. (I believe it's related to RCU domain ?) > > Hello Yann, > > Now that I've had a closer look at the code in include/rdma/ib_addr.h, > that code probably isn't safe. How about the (untested) patch below ? > Thanks for investigating. I'm not an expert in RCU, but I believe it then miss the RCU annotations around the RCU reader section (ensure correct ordering if I recall correctly). Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h > index ce55906..5a416ac 100644 > --- a/include/rdma/ib_addr.h > +++ b/include/rdma/ib_addr.h > @@ -184,7 +184,7 @@ static inline void iboe_addr_get_sgid(struct rdma_dev_addr *dev_addr, > > dev = dev_get_by_index(&init_net, dev_addr->bound_dev_if); > if (dev) { + rcu_read_lock(); > - ip4 = (struct in_device *)dev->ip_ptr; > + ip4 = __in_dev_get_rcu(dev); > if (ip4 && ip4->ifa_list && ip4->ifa_list->ifa_address) > ipv6_addr_set_v4mapped(ip4->ifa_list->ifa_address, > (struct in6_addr *)gid); + rcu_read_unlock(); Regards.
On Mon, Mar 10, 2014 at 04:02:13PM +0100, Yann Droneaud wrote: > Hi, > > Le lundi 10 mars 2014 à 15:26 +0100, Bart Van Assche a écrit : > > On 03/10/14 14:33, Yann Droneaud wrote: > > > Le lundi 10 mars 2014 à 13:22 +0100, Bart Van Assche a écrit : > > >> Suppress the following sparse warning: > > >> include/rdma/ib_addr.h:187:24: warning: cast removes address space of expression > > > > > > You should explain why there's a warning here, and why is it safe to > > > cast. (I believe it's related to RCU domain ?) > > > > Hello Yann, > > > > Now that I've had a closer look at the code in include/rdma/ib_addr.h, > > that code probably isn't safe. How about the (untested) patch below ? > > > > Thanks for investigating. > > I'm not an expert in RCU, but I believe it then miss the RCU annotations > around the RCU reader section (ensure correct ordering if I recall > correctly). > > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> If the rcu_read_lock() isn't supplied by all callers to this function, then yes, it needs to be supplied as Yann shows below. The CONFIG_PROVE_RCU=y Kconfig option can help determine that they are needed, but of course cannot prove that they are not needed, at least not unless you have a workload that exercises all possible calls to this function. Thanx, Paul > > diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h > > index ce55906..5a416ac 100644 > > --- a/include/rdma/ib_addr.h > > +++ b/include/rdma/ib_addr.h > > @@ -184,7 +184,7 @@ static inline void iboe_addr_get_sgid(struct rdma_dev_addr *dev_addr, > > > > dev = dev_get_by_index(&init_net, dev_addr->bound_dev_if); > > if (dev) { > > + rcu_read_lock(); > > > - ip4 = (struct in_device *)dev->ip_ptr; > > + ip4 = __in_dev_get_rcu(dev); > > > if (ip4 && ip4->ifa_list && ip4->ifa_list->ifa_address) > > ipv6_addr_set_v4mapped(ip4->ifa_list->ifa_address, > > (struct in6_addr *)gid); > > + rcu_read_unlock(); > > > Regards. > > -- > Yann Droneaud > OPTEYA > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/14 17:08, Paul E. McKenney wrote: > On Mon, Mar 10, 2014 at 04:02:13PM +0100, Yann Droneaud wrote: >> Hi, >> >> Le lundi 10 mars 2014 à 15:26 +0100, Bart Van Assche a écrit : >>> On 03/10/14 14:33, Yann Droneaud wrote: >>>> Le lundi 10 mars 2014 à 13:22 +0100, Bart Van Assche a écrit : >>>>> Suppress the following sparse warning: >>>>> include/rdma/ib_addr.h:187:24: warning: cast removes address space of expression >>>> >>>> You should explain why there's a warning here, and why is it safe to >>>> cast. (I believe it's related to RCU domain ?) >>> >>> Hello Yann, >>> >>> Now that I've had a closer look at the code in include/rdma/ib_addr.h, >>> that code probably isn't safe. How about the (untested) patch below ? >>> >> >> Thanks for investigating. >> >> I'm not an expert in RCU, but I believe it then miss the RCU annotations >> around the RCU reader section (ensure correct ordering if I recall >> correctly). >> >> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > If the rcu_read_lock() isn't supplied by all callers to this function, > then yes, it needs to be supplied as Yann shows below. > > The CONFIG_PROVE_RCU=y Kconfig option can help determine that they are > needed, but of course cannot prove that they are not needed, at least > not unless you have a workload that exercises all possible calls to > this function. Hello Moni, I think this warning got introduced via commit "IB/cma: IBoE (RoCE) IP-based GID addressing" (7b85627b9f02f9b0fb2ef5f021807f4251135857; December 12, 2013). Can you follow this up further ? Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/12/2014 12:15 PM, Bart Van Assche wrote: > On 03/10/14 17:08, Paul E. McKenney wrote: >> On Mon, Mar 10, 2014 at 04:02:13PM +0100, Yann Droneaud wrote: >>> Hi, >>> >>> Le lundi 10 mars 2014 à 15:26 +0100, Bart Van Assche a écrit : >>>> On 03/10/14 14:33, Yann Droneaud wrote: >>>>> Le lundi 10 mars 2014 à 13:22 +0100, Bart Van Assche a écrit : >>>>>> Suppress the following sparse warning: >>>>>> include/rdma/ib_addr.h:187:24: warning: cast removes address space of expression >>>>> You should explain why there's a warning here, and why is it safe to >>>>> cast. (I believe it's related to RCU domain ?) >>>> Hello Yann, >>>> >>>> Now that I've had a closer look at the code in include/rdma/ib_addr.h, >>>> that code probably isn't safe. How about the (untested) patch below ? >>>> >>> Thanks for investigating. >>> >>> I'm not an expert in RCU, but I believe it then miss the RCU annotations >>> around the RCU reader section (ensure correct ordering if I recall >>> correctly). >>> >>> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> >> If the rcu_read_lock() isn't supplied by all callers to this function, >> then yes, it needs to be supplied as Yann shows below. >> >> The CONFIG_PROVE_RCU=y Kconfig option can help determine that they are >> needed, but of course cannot prove that they are not needed, at least >> not unless you have a workload that exercises all possible calls to >> this function. > Hello Moni, > > I think this warning got introduced via commit "IB/cma: IBoE (RoCE) > IP-based GID addressing" (7b85627b9f02f9b0fb2ef5f021807f4251135857; > December 12, 2013). Can you follow this up further ? > > Thanks, > > Bart. Sure. I'll look into it. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/include/rdma/ib_addr.h b/include/rdma/ib_addr.h index ce55906..5a416ac 100644 --- a/include/rdma/ib_addr.h +++ b/include/rdma/ib_addr.h @@ -184,7 +184,7 @@ static inline void iboe_addr_get_sgid(struct rdma_dev_addr *dev_addr, dev = dev_get_by_index(&init_net, dev_addr->bound_dev_if); if (dev) { - ip4 = (struct in_device *)dev->ip_ptr; + ip4 = __in_dev_get_rcu(dev); if (ip4 && ip4->ifa_list && ip4->ifa_list->ifa_address) ipv6_addr_set_v4mapped(ip4->ifa_list->ifa_address, (struct in6_addr *)gid);