| Message ID | 20220802161203.622293-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ac0dbed9ba4c38ed9b5fd3a43ee4bc1f48901a34 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: seg6: initialize induction variable to first valid array index | expand |
[ cc Andrea ] On 8/2/22 10:12 AM, Nick Desaulniers wrote: > Fixes the following warnings observed when building > CONFIG_IPV6_SEG6_LWTUNNEL=y with clang: > > net/ipv6/seg6_local.o: warning: objtool: seg6_local_fill_encap() falls > through to next function seg6_local_get_encap_size() > net/ipv6/seg6_local.o: warning: objtool: seg6_local_cmp_encap() falls > through to next function input_action_end() > > LLVM can fully unroll loops in seg6_local_get_encap_size() and > seg6_local_cmp_encap(). One issue in those loops is that the induction > variable is initialized to 0. The loop iterates over members of > seg6_action_params, a global array of struct seg6_action_param calling > their put() function pointer members. seg6_action_param uses an array > initializer to initialize SEG6_LOCAL_SRH and later elements, which is > the third enumeration of an anonymous union. > > The guard `if (attrs & SEG6_F_ATTR(i))` may prevent this from being > called at runtime, but it would still be UB for > `seg6_action_params[0]->put` to be called; the unrolled loop will make > the initial iterations unreachable, which LLVM will later rotate to > fallthrough to the next function. > > Make this more obvious that this cannot happen to the compiler by > initializing the loop induction variable to the minimum valid index that > seg6_action_params is initialized to. > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > net/ipv6/seg6_local.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > index 2cd4a8d3b30a..b7de5e46fdd8 100644 > --- a/net/ipv6/seg6_local.c > +++ b/net/ipv6/seg6_local.c > @@ -1614,7 +1614,7 @@ static void __destroy_attrs(unsigned long parsed_attrs, int max_parsed, > * callback. If the callback is not available, then we skip to the next > * attribute; otherwise, we call the destroy() callback. > */ > - for (i = 0; i < max_parsed; ++i) { > + for (i = SEG6_LOCAL_SRH; i < max_parsed; ++i) { > if (!(parsed_attrs & SEG6_F_ATTR(i))) > continue; > > @@ -1643,7 +1643,7 @@ static int parse_nla_optional_attrs(struct nlattr **attrs, > struct seg6_action_param *param; > int err, i; > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; ++i) { > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; ++i) { > if (!(desc->optattrs & SEG6_F_ATTR(i)) || !attrs[i]) > continue; > > @@ -1742,7 +1742,7 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt) > } > > /* parse the required attributes */ > - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { > if (desc->attrs & SEG6_F_ATTR(i)) { > if (!attrs[i]) > return -EINVAL; > @@ -1847,7 +1847,7 @@ static int seg6_local_fill_encap(struct sk_buff *skb, > > attrs = slwt->desc->attrs | slwt->parsed_optattrs; > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { > if (attrs & SEG6_F_ATTR(i)) { > param = &seg6_action_params[i]; > err = param->put(skb, slwt); > @@ -1927,7 +1927,7 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a, > if (attrs_a != attrs_b) > return 1; > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { > if (attrs_a & SEG6_F_ATTR(i)) { > param = &seg6_action_params[i]; > if (param->cmp(slwt_a, slwt_b))
Hi Nick, please see below, thanks. > [ cc Andrea ] @David Ahern: thanks David! > On 8/2/22 10:12 AM, Nick Desaulniers wrote: > > Fixes the following warnings observed when building > > CONFIG_IPV6_SEG6_LWTUNNEL=y with clang: > > > > net/ipv6/seg6_local.o: warning: objtool: seg6_local_fill_encap() falls > > through to next function seg6_local_get_encap_size() > > net/ipv6/seg6_local.o: warning: objtool: seg6_local_cmp_encap() falls > > through to next function input_action_end() > > > > LLVM can fully unroll loops in seg6_local_get_encap_size() and > > seg6_local_cmp_encap(). One issue in those loops is that the induction > > variable is initialized to 0. The loop iterates over members of > > seg6_action_params, a global array of struct seg6_action_param calling > > their put() function pointer members. seg6_action_param uses an array > > initializer to initialize SEG6_LOCAL_SRH and later elements, which is > > the third enumeration of an anonymous union. > > > > The guard `if (attrs & SEG6_F_ATTR(i))` may prevent this from being > > called at runtime, but it would still be UB for > > `seg6_action_params[0]->put` to be called; the unrolled loop will make > > the initial iterations unreachable, which LLVM will later rotate to > > fallthrough to the next function. > > > > Make this more obvious that this cannot happen to the compiler by > > initializing the loop induction variable to the minimum valid index that > > seg6_action_params is initialized to. By looking at the patch, it looks that your changes seem reasonable. The first two SEG6_LOCAL_{UNSPEC,ACTION} attributes are quite "special" in the sense that they do not have the seg6_action_param callbacks set. The SEG6_LOCAL_UNSPEC attribute is practically not used; while SEG6_LOCAL_ACTION is handled out of the several loops. Hence, I think we can skip these two attributes (in the loops) by setting the induction variable to the value you proposed, i.e. SEG6_LOCAL_SRH. Ciao, Andrea > > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > net/ipv6/seg6_local.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > > index 2cd4a8d3b30a..b7de5e46fdd8 100644 > > --- a/net/ipv6/seg6_local.c > > +++ b/net/ipv6/seg6_local.c > > @@ -1614,7 +1614,7 @@ static void __destroy_attrs(unsigned long parsed_attrs, int max_parsed, > > * callback. If the callback is not available, then we skip to the next > > * attribute; otherwise, we call the destroy() callback. > > */ > > - for (i = 0; i < max_parsed; ++i) { > > + for (i = SEG6_LOCAL_SRH; i < max_parsed; ++i) { > > if (!(parsed_attrs & SEG6_F_ATTR(i))) > > continue; > > > > @@ -1643,7 +1643,7 @@ static int parse_nla_optional_attrs(struct nlattr **attrs, > > struct seg6_action_param *param; > > int err, i; > > > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; ++i) { > > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; ++i) { > > if (!(desc->optattrs & SEG6_F_ATTR(i)) || !attrs[i]) > > continue; > > > > @@ -1742,7 +1742,7 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt) > > } > > > > /* parse the required attributes */ > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { > > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { > > if (desc->attrs & SEG6_F_ATTR(i)) { > > if (!attrs[i]) > > return -EINVAL; > > @@ -1847,7 +1847,7 @@ static int seg6_local_fill_encap(struct sk_buff *skb, > > > > attrs = slwt->desc->attrs | slwt->parsed_optattrs; > > > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { > > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { > > if (attrs & SEG6_F_ATTR(i)) { > > param = &seg6_action_params[i]; > > err = param->put(skb, slwt); > > @@ -1927,7 +1927,7 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a, > > if (attrs_a != attrs_b) > > return 1; > > > > - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { > > + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { > > if (attrs_a & SEG6_F_ATTR(i)) { > > param = &seg6_action_params[i]; > > if (param->cmp(slwt_a, slwt_b)) >
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 2 Aug 2022 09:12:03 -0700 you wrote: > Fixes the following warnings observed when building > CONFIG_IPV6_SEG6_LWTUNNEL=y with clang: > > net/ipv6/seg6_local.o: warning: objtool: seg6_local_fill_encap() falls > through to next function seg6_local_get_encap_size() > net/ipv6/seg6_local.o: warning: objtool: seg6_local_cmp_encap() falls > through to next function input_action_end() > > [...] Here is the summary with links: - net: seg6: initialize induction variable to first valid array index https://git.kernel.org/netdev/net/c/ac0dbed9ba4c You are awesome, thank you!
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c index 2cd4a8d3b30a..b7de5e46fdd8 100644 --- a/net/ipv6/seg6_local.c +++ b/net/ipv6/seg6_local.c @@ -1614,7 +1614,7 @@ static void __destroy_attrs(unsigned long parsed_attrs, int max_parsed, * callback. If the callback is not available, then we skip to the next * attribute; otherwise, we call the destroy() callback. */ - for (i = 0; i < max_parsed; ++i) { + for (i = SEG6_LOCAL_SRH; i < max_parsed; ++i) { if (!(parsed_attrs & SEG6_F_ATTR(i))) continue; @@ -1643,7 +1643,7 @@ static int parse_nla_optional_attrs(struct nlattr **attrs, struct seg6_action_param *param; int err, i; - for (i = 0; i < SEG6_LOCAL_MAX + 1; ++i) { + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; ++i) { if (!(desc->optattrs & SEG6_F_ATTR(i)) || !attrs[i]) continue; @@ -1742,7 +1742,7 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt) } /* parse the required attributes */ - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { if (desc->attrs & SEG6_F_ATTR(i)) { if (!attrs[i]) return -EINVAL; @@ -1847,7 +1847,7 @@ static int seg6_local_fill_encap(struct sk_buff *skb, attrs = slwt->desc->attrs | slwt->parsed_optattrs; - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { if (attrs & SEG6_F_ATTR(i)) { param = &seg6_action_params[i]; err = param->put(skb, slwt); @@ -1927,7 +1927,7 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a, if (attrs_a != attrs_b) return 1; - for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) { + for (i = SEG6_LOCAL_SRH; i < SEG6_LOCAL_MAX + 1; i++) { if (attrs_a & SEG6_F_ATTR(i)) { param = &seg6_action_params[i]; if (param->cmp(slwt_a, slwt_b))
Fixes the following warnings observed when building CONFIG_IPV6_SEG6_LWTUNNEL=y with clang: net/ipv6/seg6_local.o: warning: objtool: seg6_local_fill_encap() falls through to next function seg6_local_get_encap_size() net/ipv6/seg6_local.o: warning: objtool: seg6_local_cmp_encap() falls through to next function input_action_end() LLVM can fully unroll loops in seg6_local_get_encap_size() and seg6_local_cmp_encap(). One issue in those loops is that the induction variable is initialized to 0. The loop iterates over members of seg6_action_params, a global array of struct seg6_action_param calling their put() function pointer members. seg6_action_param uses an array initializer to initialize SEG6_LOCAL_SRH and later elements, which is the third enumeration of an anonymous union. The guard `if (attrs & SEG6_F_ATTR(i))` may prevent this from being called at runtime, but it would still be UB for `seg6_action_params[0]->put` to be called; the unrolled loop will make the initial iterations unreachable, which LLVM will later rotate to fallthrough to the next function. Make this more obvious that this cannot happen to the compiler by initializing the loop induction variable to the minimum valid index that seg6_action_params is initialized to. Reported-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- net/ipv6/seg6_local.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)