| Message ID | 1455550923-23673-2-git-send-email-rshearma@brocade.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 15 Feb 2016 15:42:01 +0000, Robert Shearman wrote: > +static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type) > +{ > + switch (encap_type) { > + case LWTUNNEL_ENCAP_MPLS: > + return "LWTUNNEL_ENCAP_MPLS"; > + case LWTUNNEL_ENCAP_IP: > + return "LWTUNNEL_ENCAP_IP"; > + case LWTUNNEL_ENCAP_ILA: > + return "LWTUNNEL_ENCAP_ILA"; > + case LWTUNNEL_ENCAP_IP6: > + return "LWTUNNEL_ENCAP_IP6"; > + case LWTUNNEL_ENCAP_NONE: > + case __LWTUNNEL_ENCAP_MAX: > + /* should not have got here */ > + break; > + } > + WARN_ON(1); > + return "LWTUNNEL_ENCAP_NONE"; > +} > + > +#endif /* CONFIG_MODULES */ > + > struct lwtunnel_state *lwtunnel_state_alloc(int encap_len) > { > struct lwtunnel_state *lws; > @@ -85,6 +109,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, > ret = -EOPNOTSUPP; > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[encap_type]); > +#ifdef CONFIG_MODULES > + if (!ops) { > + rcu_read_unlock(); > + request_module("rtnl-lwt-%s", lwtunnel_encap_str(encap_type)); Why the repeating of "lwt"/"LWTUNNEL" and the unnecessary "ENCAP"? Wouldn't be lwtunnel_encap_str returning just "MPLS" or "ILA" enough? I don't have any strong preference here, it just looks weird to me. Maybe there's a reason. Also, this doesn't affect IP lwtunnels, i.e. LWTUNNEL_ENCAP_IP and LWTUNNEL_ENCAP_IP6. Should we just return NULL from lwtunnel_encap_str in such cases (plus unknown encap_type) and WARN on the NULL here? Jiri
On 15/02/16 16:02, Jiri Benc wrote: > On Mon, 15 Feb 2016 15:42:01 +0000, Robert Shearman wrote: >> +static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type) >> +{ >> + switch (encap_type) { >> + case LWTUNNEL_ENCAP_MPLS: >> + return "LWTUNNEL_ENCAP_MPLS"; >> + case LWTUNNEL_ENCAP_IP: >> + return "LWTUNNEL_ENCAP_IP"; >> + case LWTUNNEL_ENCAP_ILA: >> + return "LWTUNNEL_ENCAP_ILA"; >> + case LWTUNNEL_ENCAP_IP6: >> + return "LWTUNNEL_ENCAP_IP6"; >> + case LWTUNNEL_ENCAP_NONE: >> + case __LWTUNNEL_ENCAP_MAX: >> + /* should not have got here */ >> + break; >> + } >> + WARN_ON(1); >> + return "LWTUNNEL_ENCAP_NONE"; >> +} >> + >> +#endif /* CONFIG_MODULES */ >> + >> struct lwtunnel_state *lwtunnel_state_alloc(int encap_len) >> { >> struct lwtunnel_state *lws; >> @@ -85,6 +109,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, >> ret = -EOPNOTSUPP; >> rcu_read_lock(); >> ops = rcu_dereference(lwtun_encaps[encap_type]); >> +#ifdef CONFIG_MODULES >> + if (!ops) { >> + rcu_read_unlock(); >> + request_module("rtnl-lwt-%s", lwtunnel_encap_str(encap_type)); > > Why the repeating of "lwt"/"LWTUNNEL" and the unnecessary "ENCAP"? > Wouldn't be lwtunnel_encap_str returning just "MPLS" or "ILA" enough? > I don't have any strong preference here, it just looks weird to me. > Maybe there's a reason. Yeah, it's the C preprocessor. MODULE_ALIAS_RTNL_LWT includes the string for the encap type in the module alias, and since the LWT encap types are defined as enums this is symbolic name. I can't see any way of getting the preprocessor to convert MODULE_ALIAS_RTNL_LWT(LWTUNNEL_ENCAP_MPLS) into "rtnl-lwt-MPLS", but I'm open to suggestions. I could just drop the "lwt-" bit of the alias string given that it's included in the name of the enum values. > Also, this doesn't affect IP lwtunnels, i.e. LWTUNNEL_ENCAP_IP and > LWTUNNEL_ENCAP_IP6. Should we just return NULL from lwtunnel_encap_str > in such cases (plus unknown encap_type) and WARN on the NULL here? True, but I figured that it was cleaner for the lwtunnel infra to not assume whether how those modules are implemented. If you disagree, then I can change to doing as you suggest. Thanks, Rob
On Mon, 15 Feb 2016 16:22:08 +0000, Robert Shearman wrote: > Yeah, it's the C preprocessor. MODULE_ALIAS_RTNL_LWT includes the string > for the encap type in the module alias, and since the LWT encap types > are defined as enums this is symbolic name. I can't see any way of > getting the preprocessor to convert > MODULE_ALIAS_RTNL_LWT(LWTUNNEL_ENCAP_MPLS) into "rtnl-lwt-MPLS", but I'm > open to suggestions. MODULE_ALIAS_RTNL_LWT(MPLS)? But whatever, as I said, no strong preference. > True, but I figured that it was cleaner for the lwtunnel infra to not > assume whether how those modules are implemented. If you disagree, then > I can change to doing as you suggest. It's not completely transparent to the infrastructure anyway, the tunnel type needs to be added to lwtunnel_encap_str for new tunnels. The way I suggested, it's only added for those tunnels having the module alias set. Just trying to get rid of the unnecessary strings in lwtunnel_encap_str. There's no need to bloat kernel with them if they're never used. Jiri
On 15/02/16 16:32, Jiri Benc wrote: > On Mon, 15 Feb 2016 16:22:08 +0000, Robert Shearman wrote: >> Yeah, it's the C preprocessor. MODULE_ALIAS_RTNL_LWT includes the string >> for the encap type in the module alias, and since the LWT encap types >> are defined as enums this is symbolic name. I can't see any way of >> getting the preprocessor to convert >> MODULE_ALIAS_RTNL_LWT(LWTUNNEL_ENCAP_MPLS) into "rtnl-lwt-MPLS", but I'm >> open to suggestions. > > MODULE_ALIAS_RTNL_LWT(MPLS)? > > But whatever, as I said, no strong preference. I was so hung up on the making the string match the name of the enum that I'd discounted that, but you're right that doing that would reduce duplication in the alias string. >> True, but I figured that it was cleaner for the lwtunnel infra to not >> assume whether how those modules are implemented. If you disagree, then >> I can change to doing as you suggest. > > It's not completely transparent to the infrastructure anyway, the > tunnel type needs to be added to lwtunnel_encap_str for new tunnels. > The way I suggested, it's only added for those tunnels having the > module alias set. > > Just trying to get rid of the unnecessary strings in > lwtunnel_encap_str. There's no need to bloat kernel with them if > they're never used. Ok, will resubmit without the unnecessary strings in that function as well as with your suggestion above. Thanks for the review, Rob
Robert Shearman <rshearma@brocade.com> writes: > The lwt implementations using net devices can autoload using the > existing mechanism using IFLA_INFO_KIND. However, there's no mechanism > that lwt modules not using net devices can use. > > Therefore, add the ability to autoload modules registering lwt > operations for lwt implementations not using a net device so that > users don't have to manually load the modules. > > Signed-off-by: Robert Shearman <rshearma@brocade.com> > --- > include/net/lwtunnel.h | 4 +++- > net/core/lwtunnel.c | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h > index 66350ce3e955..e9f116e29c22 100644 > --- a/include/net/lwtunnel.h > +++ b/include/net/lwtunnel.h > @@ -170,6 +170,8 @@ static inline int lwtunnel_input(struct sk_buff *skb) > return -EOPNOTSUPP; > } > > -#endif > +#endif /* CONFIG_LWTUNNEL */ > + > +#define MODULE_ALIAS_RTNL_LWT(encap_type) MODULE_ALIAS("rtnl-lwt-" __stringify(encap_type)) > > #endif /* __NET_LWTUNNEL_H */ > diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c > index 299cfc24d888..8ef5e5cec03e 100644 > --- a/net/core/lwtunnel.c > +++ b/net/core/lwtunnel.c > @@ -27,6 +27,30 @@ > #include <net/rtnetlink.h> > #include <net/ip6_fib.h> > > +#ifdef CONFIG_MODULES > + > +static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type) > +{ > + switch (encap_type) { > + case LWTUNNEL_ENCAP_MPLS: > + return "LWTUNNEL_ENCAP_MPLS"; > + case LWTUNNEL_ENCAP_IP: > + return "LWTUNNEL_ENCAP_IP"; > + case LWTUNNEL_ENCAP_ILA: > + return "LWTUNNEL_ENCAP_ILA"; > + case LWTUNNEL_ENCAP_IP6: > + return "LWTUNNEL_ENCAP_IP6"; > + case LWTUNNEL_ENCAP_NONE: > + case __LWTUNNEL_ENCAP_MAX: > + /* should not have got here */ > + break; > + } > + WARN_ON(1); > + return "LWTUNNEL_ENCAP_NONE"; > +} > + > +#endif /* CONFIG_MODULES */ > + > struct lwtunnel_state *lwtunnel_state_alloc(int encap_len) > { > struct lwtunnel_state *lws; > @@ -85,6 +109,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, > ret = -EOPNOTSUPP; > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[encap_type]); > +#ifdef CONFIG_MODULES > + if (!ops) { > + rcu_read_unlock(); > + request_module("rtnl-lwt-%s", lwtunnel_encap_str(encap_type)); > + rcu_read_lock(); > + ops = rcu_dereference(lwtun_encaps[encap_type]); > + } > +#endif > if (likely(ops && ops->build_state)) > ret = ops->build_state(dev, encap, family, cfg, lws); > rcu_read_unlock(); My memory is fuzzy on how this is done elsewhere but this looks like it needs a capability check to ensure that non-root user's can't trigger this. It tends to be problematic if a non-root user can trigger an autoload of a known-buggy module. With a combination of user namespaces and network namespaces unprivileged users can cause just about every corner of the network stack to be exercised. Eric
On 15/02/16 21:33, Eric W. Biederman wrote: > Robert Shearman <rshearma@brocade.com> writes: >> @@ -85,6 +109,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, >> ret = -EOPNOTSUPP; >> rcu_read_lock(); >> ops = rcu_dereference(lwtun_encaps[encap_type]); >> +#ifdef CONFIG_MODULES >> + if (!ops) { >> + rcu_read_unlock(); >> + request_module("rtnl-lwt-%s", lwtunnel_encap_str(encap_type)); >> + rcu_read_lock(); >> + ops = rcu_dereference(lwtun_encaps[encap_type]); >> + } >> +#endif >> if (likely(ops && ops->build_state)) >> ret = ops->build_state(dev, encap, family, cfg, lws); >> rcu_read_unlock(); > > My memory is fuzzy on how this is done elsewhere but this looks like it > needs a capability check to ensure that non-root user's can't trigger > this. > > It tends to be problematic if a non-root user can trigger an autoload of > a known-buggy module. With a combination of user namespaces and network > namespaces unprivileged users can cause just about every corner of the > network stack to be exercised. The same protections apply to this as to the IFLA_INFO_KIND module autoloading, namely by rtnetlink_rcv_msg ensuring that no messages other than gets can be done by an unprivileged user: type = nlh->nlmsg_type; ... type -= RTM_BASE; ... kind = type&3; if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; The lwtunnel_build_state function is only called by the processing of non-get message types. Is this sufficient or are you looking for something in addition? Thanks, Rob
diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h index 66350ce3e955..e9f116e29c22 100644 --- a/include/net/lwtunnel.h +++ b/include/net/lwtunnel.h @@ -170,6 +170,8 @@ static inline int lwtunnel_input(struct sk_buff *skb) return -EOPNOTSUPP; } -#endif +#endif /* CONFIG_LWTUNNEL */ + +#define MODULE_ALIAS_RTNL_LWT(encap_type) MODULE_ALIAS("rtnl-lwt-" __stringify(encap_type)) #endif /* __NET_LWTUNNEL_H */ diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c index 299cfc24d888..8ef5e5cec03e 100644 --- a/net/core/lwtunnel.c +++ b/net/core/lwtunnel.c @@ -27,6 +27,30 @@ #include <net/rtnetlink.h> #include <net/ip6_fib.h> +#ifdef CONFIG_MODULES + +static const char *lwtunnel_encap_str(enum lwtunnel_encap_types encap_type) +{ + switch (encap_type) { + case LWTUNNEL_ENCAP_MPLS: + return "LWTUNNEL_ENCAP_MPLS"; + case LWTUNNEL_ENCAP_IP: + return "LWTUNNEL_ENCAP_IP"; + case LWTUNNEL_ENCAP_ILA: + return "LWTUNNEL_ENCAP_ILA"; + case LWTUNNEL_ENCAP_IP6: + return "LWTUNNEL_ENCAP_IP6"; + case LWTUNNEL_ENCAP_NONE: + case __LWTUNNEL_ENCAP_MAX: + /* should not have got here */ + break; + } + WARN_ON(1); + return "LWTUNNEL_ENCAP_NONE"; +} + +#endif /* CONFIG_MODULES */ + struct lwtunnel_state *lwtunnel_state_alloc(int encap_len) { struct lwtunnel_state *lws; @@ -85,6 +109,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, ret = -EOPNOTSUPP; rcu_read_lock(); ops = rcu_dereference(lwtun_encaps[encap_type]); +#ifdef CONFIG_MODULES + if (!ops) { + rcu_read_unlock(); + request_module("rtnl-lwt-%s", lwtunnel_encap_str(encap_type)); + rcu_read_lock(); + ops = rcu_dereference(lwtun_encaps[encap_type]); + } +#endif if (likely(ops && ops->build_state)) ret = ops->build_state(dev, encap, family, cfg, lws); rcu_read_unlock();
The lwt implementations using net devices can autoload using the existing mechanism using IFLA_INFO_KIND. However, there's no mechanism that lwt modules not using net devices can use. Therefore, add the ability to autoload modules registering lwt operations for lwt implementations not using a net device so that users don't have to manually load the modules. Signed-off-by: Robert Shearman <rshearma@brocade.com> --- include/net/lwtunnel.h | 4 +++- net/core/lwtunnel.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-)