| Message ID | 20241017174109.85717-10-stefan.wiehler@nokia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Lock RCU before calling ip6mr_get_table() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | fail | Patch does not apply to net-0 |
Stefan Wiehler <stefan.wiehler@nokia.com> wrote: > When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables > must be read under RCU or RTNL lock. > > Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables") > Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com> > --- > net/ipv6/ip6mr.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c > index b169b27de7e1..39aac81a30f1 100644 > --- a/net/ipv6/ip6mr.c > +++ b/net/ipv6/ip6mr.c > @@ -2633,27 +2633,31 @@ static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > grp = nla_get_in6_addr(tb[RTA_DST]); > tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; > > + rcu_read_lock(); AFAICS ip6mr_rtm_getroute() runs with RTNL held, so I don't see why this patch is needed.
>> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables >> must be read under RCU or RTNL lock. >> >> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables") >> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com> >> --- >> net/ipv6/ip6mr.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c >> index b169b27de7e1..39aac81a30f1 100644 >> --- a/net/ipv6/ip6mr.c >> +++ b/net/ipv6/ip6mr.c >> @@ -2633,27 +2633,31 @@ static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, >> grp = nla_get_in6_addr(tb[RTA_DST]); >> tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; >> >> + rcu_read_lock(); > > AFAICS ip6mr_rtm_getroute() runs with RTNL held, so I don't see > why this patch is needed. That's true, but it's called neither with RCU nor RTNL lock when RTNL_FLAG_DOIT_UNLOCKED is set in rtnetlink_rcv_msg(): > if (flags & RTNL_FLAG_DOIT_UNLOCKED) { > doit = link->doit; > rcu_read_unlock(); > if (doit) > err = doit(skb, nlh, extack); > module_put(owner); > return err; > } > rcu_read_unlock(); I realized now that I completely misunderstood how ip6mr_rtm_dumproute() gets called - it should be still safe though because mpls_netconf_dump_devconf() and getaddr_dumpit() hold the RCU lock while mpls_dump_routes() asserts that the RTNL lock is held. Is that observation correct?
Stefan Wiehler <stefan.wiehler@nokia.com> wrote: > >> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables > >> must be read under RCU or RTNL lock. > >> > >> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables") > >> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com> > >> --- > >> net/ipv6/ip6mr.c | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c > >> index b169b27de7e1..39aac81a30f1 100644 > >> --- a/net/ipv6/ip6mr.c > >> +++ b/net/ipv6/ip6mr.c > >> @@ -2633,27 +2633,31 @@ static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, > >> grp = nla_get_in6_addr(tb[RTA_DST]); > >> tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; > >> > >> + rcu_read_lock(); > > > > AFAICS ip6mr_rtm_getroute() runs with RTNL held, so I don't see > > why this patch is needed. > > That's true, but it's called neither with RCU nor RTNL lock when > RTNL_FLAG_DOIT_UNLOCKED is set in rtnetlink_rcv_msg(): Sure, but RTNL_FLAG_DOIT_UNLOCKED is not set for this function: err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE, ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0); (0 == flag field). So RNTL is held. Would of course be nice to convert it to RCU eventually but thats an enhancement, not a bug fix, so this must be in separate changesets, targetting net and net-next, respectively. > I realized now that I completely misunderstood how ip6mr_rtm_dumproute() gets > called - it should be still safe though because mpls_netconf_dump_devconf() and > getaddr_dumpit() hold the RCU lock while mpls_dump_routes() asserts that the > RTNL lock is held. Is that observation correct? {THIS_MODULE, PF_PHONET, RTM_GETADDR, NULL, getaddr_dumpit, 0}, {THIS_MODULE, PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, 0}, Both get called with RTNL mutex held, but not within an RCU read side section. but: {THIS_MODULE, PF_MPLS, RTM_GETNETCONF, [..] mpls_netconf_dump_devconf, RTNL_FLAG_DUMP_UNLOCKED}, // == no RTNL held Means: dump callback is invoked without RTNL mutex. Those functions are also called without and RCU read-side section. Both the get (doit) and the dumper function callbacks need to explicitly opt-in for RTNL-less invocation at register time.
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c index b169b27de7e1..39aac81a30f1 100644 --- a/net/ipv6/ip6mr.c +++ b/net/ipv6/ip6mr.c @@ -2633,27 +2633,31 @@ static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, grp = nla_get_in6_addr(tb[RTA_DST]); tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0; + rcu_read_lock(); mrt = ip6mr_get_table(net, tableid ?: RT_TABLE_DEFAULT); if (!mrt) { + rcu_read_unlock(); NL_SET_ERR_MSG_MOD(extack, "MR table does not exist"); return -ENOENT; } /* entries are added/deleted only under RTNL */ - rcu_read_lock(); cache = ip6mr_cache_find(mrt, &src, &grp); - rcu_read_unlock(); if (!cache) { + rcu_read_unlock(); NL_SET_ERR_MSG_MOD(extack, "MR cache entry not found"); return -ENOENT; } skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL); - if (!skb) + if (!skb) { + rcu_read_unlock(); return -ENOBUFS; + } err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0); + rcu_read_unlock(); if (err < 0) { kfree_skb(skb); return err;
When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables must be read under RCU or RTNL lock. Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables") Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com> --- net/ipv6/ip6mr.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)