Hi,
On Sa, 2014-08-30 at 12:58 +0200, Sabrina Dubroca wrote:
> 2014-08-30, 03:51:29 +0200, Hannes Frederic Sowa wrote:
> > Hi Sabrina,
> >
> > [...]
> >
> > Sorry, just had time to look at this.
> >
> > The reason is not to have list corruption but that the calls down to
> > ndo_set_rx_mode expect rtnl to be locked by the drivers. Filter lists
> > are locked by addr_list_lock and that's why I think we never saw any
> > problems with that, but drivers expect rtnl locked for those calls.
> >
> > But this problem also affects multicast join, so patch seems incomplete
> > to me (and for that matter ssm multicast join, too).
> >
> > Also rtnl_lock and rcu_read_lock compose in that order, so we don't need
> > to change dev_get_by_flags, but as this is the only user it sure is
> > possible. RCU locked version is just easier composeable, so I wouldn't
> > touch that if needed in future, just also take rcu lock as before.
> >
> > So just adding rtnl_lock add appropriate places seems to be ok to me,
> > but still need to review parts of the ssm code.
> >
> > Also we should move ASSERT_RTNL checks from addrconf_join_solict to
> > ipv6_dev_mc_inc/dec.
> >
> > Thanks,
> > Hannes
>
> Thanks for explaining.
>
> I had a look at what you suggested.
>
>
> So, for anycast, on top of the previous patch, we'd have:
>
> ---
> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> index 210183244689..61dd3046b804 100644
> --- a/net/ipv6/anycast.c
> +++ b/net/ipv6/anycast.c
> static void aca_put(struct ifacaddr6 *ac)
> @@ -233,6 +235,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
> struct rt6_info *rt;
> int err;
>
> + ASSERT_RTNL();
> +
> idev = in6_dev_get(dev);
>
> if (idev == NULL)
> @@ -302,6 +306,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
> {
> struct ifacaddr6 *aca, *prev_aca;
>
> + ASSERT_RTNL();
> +
> write_lock_bh(&idev->lock);
> prev_aca = NULL;
> for (aca = idev->ac_list; aca; aca = aca->aca_next) {
> @@ -336,6 +342,8 @@ static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
> {
> struct inet6_dev *idev = __in6_dev_get(dev);
>
> + ASSERT_RTNL();
> +
> if (idev == NULL)
> return -ENODEV;
> return __ipv6_dev_ac_dec(idev, addr);
ASSERT_RTNL() still performs a runtime check. While those are not really
fast paths, I still think it is better to keep them to a minimum and
place them only at places where we know all code which needs to be
guarded passes by:
I would suggest to move ASSERT_RTNL to ipv6_dev_mc_inc and
__ipv6_dev_mc_dec and even remove the checks from addrconf_join_solict
and addrconf_leave_solict. Does that cover all code paths and makes
sense?
> ---
>
>
> And for multicast:
> - locking order in the patch below: rtnl -> rcu -> ipv6_sk_mc_lock
> - ipv6_sock_mc_join: maybe move all the _unlock()'s together at the end of the function
> - do we need to modify rcu_dereference_protected in ipv6_sock_mc_drop/ipv6_sock_mc_close
> - I had a look at the other codepaths that call ipv6_dev_mc_inc/dec
> - ipv6_mc_destroy_dev, dev_forward_change, ipv6_add_dev,
> addrconf_join_solict -- all take rtnl or already have an
> ASSERT_RTNL()
> - pndisc_destructor, called from pneigh_ifdown/pneigh_delete
> - pndisc_constructor, called from pneigh_lookup -- pneigh_lookup
> has ASSERT_RTNL(), but pneigh_lookup is called from ip6_forward and
> ndisc_recv_na
> - (hope I didn't miss any callers)
>
> As far as I could see, apart maybe from pndisc_constructor, it seems
> okay, but I'd like to hear your comments.
The rest of the patch looks good.
Can you or Cong post a final patch with the adapted ac_join/drop
changes?
Thanks,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe trinity" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
