| Message ID | 1520667645-21975-1-git-send-email-andreaschristofo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 10 Mar 2018 09:40:44 +0200 Andreas Christoforou <andreaschristofo@gmail.com> wrote: > diff --git a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c > index b15075a..270a53a 100644 > --- a/net/ipv6/xfrm6_state.c > +++ b/net/ipv6/xfrm6_state.c > @@ -62,7 +62,7 @@ __xfrm6_sort(void **dst, void **src, int n, int (*cmp)(void *p), int maxclass) > { > int i; > int class[XFRM_MAX_DEPTH]; > - int count[maxclass]; > + int count[XFRM_MAX_DEPTH]; > > memset(count, 0, sizeof(count)); Can you perhaps initialize 'count' instead of calling memset(), now?
On Sat, Mar 10, 2018 at 12:43 AM, Stefano Brivio <sbrivio@redhat.com> wrote: > On Sat, 10 Mar 2018 09:40:44 +0200 > Andreas Christoforou <andreaschristofo@gmail.com> wrote: > >> diff --git a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c >> index b15075a..270a53a 100644 >> --- a/net/ipv6/xfrm6_state.c >> +++ b/net/ipv6/xfrm6_state.c >> @@ -62,7 +62,7 @@ __xfrm6_sort(void **dst, void **src, int n, int (*cmp)(void *p), int maxclass) >> { >> int i; >> int class[XFRM_MAX_DEPTH]; >> - int count[maxclass]; >> + int count[XFRM_MAX_DEPTH]; >> >> memset(count, 0, sizeof(count)); > > Can you perhaps initialize 'count' instead of calling memset(), now? Do you mean: int count[XFRM_MAX_DEPTH] = { }; instead of the memset()? I thought the compiler would resolve these both to the same thing? The former looks better though! :) -Kees
On Sat, 10 Mar 2018 09:18:46 -0800 Kees Cook <keescook@chromium.org> wrote: > On Sat, Mar 10, 2018 at 12:43 AM, Stefano Brivio <sbrivio@redhat.com> wrote: > > On Sat, 10 Mar 2018 09:40:44 +0200 > > Andreas Christoforou <andreaschristofo@gmail.com> wrote: > > > >> diff --git a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c > >> index b15075a..270a53a 100644 > >> --- a/net/ipv6/xfrm6_state.c > >> +++ b/net/ipv6/xfrm6_state.c > >> @@ -62,7 +62,7 @@ __xfrm6_sort(void **dst, void **src, int n, int (*cmp)(void *p), int maxclass) > >> { > >> int i; > >> int class[XFRM_MAX_DEPTH]; > >> - int count[maxclass]; > >> + int count[XFRM_MAX_DEPTH]; > >> > >> memset(count, 0, sizeof(count)); > > > > Can you perhaps initialize 'count' instead of calling memset(), now? > > Do you mean: > > int count[XFRM_MAX_DEPTH] = { }; > > instead of the memset()? Yep. > I thought the compiler would resolve these both to the same thing? Yes, for all practical purposes. With gcc 7.3.0 for x86_64, starting from -O1, it's exactly the same. With e.g. gcc 4.4.7, even with -O3, they can be a bit different depending on context. > The former looks better though! :) Yep! :)
On Sat, Mar 10, 2018 at 07:26:44PM +0100, Stefano Brivio wrote: > On Sat, 10 Mar 2018 09:18:46 -0800 > Kees Cook <keescook@chromium.org> wrote: > > > On Sat, Mar 10, 2018 at 12:43 AM, Stefano Brivio <sbrivio@redhat.com> wrote: > > > On Sat, 10 Mar 2018 09:40:44 +0200 > > > Andreas Christoforou <andreaschristofo@gmail.com> wrote: > > > > > >> diff --git a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c > > >> index b15075a..270a53a 100644 > > >> --- a/net/ipv6/xfrm6_state.c > > >> +++ b/net/ipv6/xfrm6_state.c > > >> @@ -62,7 +62,7 @@ __xfrm6_sort(void **dst, void **src, int n, int (*cmp)(void *p), int maxclass) > > >> { > > >> int i; > > >> int class[XFRM_MAX_DEPTH]; > > >> - int count[maxclass]; > > >> + int count[XFRM_MAX_DEPTH]; > > >> > > >> memset(count, 0, sizeof(count)); > > > > > > Can you perhaps initialize 'count' instead of calling memset(), now? > > > > Do you mean: > > > > int count[XFRM_MAX_DEPTH] = { }; > > > > instead of the memset()? > > Yep. > > > I thought the compiler would resolve these both to the same thing? > > Yes, for all practical purposes. With gcc 7.3.0 for x86_64, starting > from -O1, it's exactly the same. With e.g. gcc 4.4.7, even with -O3, > they can be a bit different depending on context. > > > The former looks better though! :) > > Yep! :) If Andreas does a v3 anyway, please also consider to trim the subject line to something like: xfrm: remove VLA usage in __xfrm6_sort()
Andreas, On Sat, 10 Mar 2018 09:40:44 +0200 Andreas Christoforou <andreaschristofo@gmail.com> wrote: > The kernel would like to have all stack VLA usage removed[1]. > Instead of dynamic allocation, just use XFRM_MAX_DEPTH > as already done for the "class" array, but as per feedback, > I will not drop maxclass because that changes the behavior. > In one case, it'll do this loop up to 5, the other > caller up to 6. > > [1] https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> > --- > v2: > - use XFRM_MAX_DEPTH for "count" array (Steffen and Mathias). > --- > net/ipv6/xfrm6_state.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c > index b15075a..270a53a 100644 > --- a/net/ipv6/xfrm6_state.c > +++ b/net/ipv6/xfrm6_state.c > @@ -62,7 +62,7 @@ __xfrm6_sort(void **dst, void **src, int n, int (*cmp)(void *p), int maxclass) > { > int i; > int class[XFRM_MAX_DEPTH]; > - int count[maxclass]; > + int count[XFRM_MAX_DEPTH]; > > memset(count, 0, sizeof(count)); > I hope this didn't get too confusing. In the end, the change I proposed for this patch was simply to drop the memset and initialize 'count' like: int count[XFRM_MAX_DEPTH] = { }; and perhaps, while at it, move this before 'int i', for coding style reasons. When you re-post, please also take care of Steffen's comment. He proposed to change the subject to: xfrm: remove VLA usage in __xfrm6_sort() Note that you should give an indication of which tree this patch should be applied to, by including this in the subject. The current subject doesn't specify it, it should have been: [PATCH v2 ipsec-next] ... Please see Documentation/networking/netdev-FAQ.txt for the difference between net and net-next, as the same distinction applies for ipsec and ipsec-next trees. Thanks.
diff --git a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c index b15075a..270a53a 100644 --- a/net/ipv6/xfrm6_state.c +++ b/net/ipv6/xfrm6_state.c @@ -62,7 +62,7 @@ __xfrm6_sort(void **dst, void **src, int n, int (*cmp)(void *p), int maxclass) { int i; int class[XFRM_MAX_DEPTH]; - int count[maxclass]; + int count[XFRM_MAX_DEPTH]; memset(count, 0, sizeof(count));
The kernel would like to have all stack VLA usage removed[1]. Instead of dynamic allocation, just use XFRM_MAX_DEPTH as already done for the "class" array, but as per feedback, I will not drop maxclass because that changes the behavior. In one case, it'll do this loop up to 5, the other caller up to 6. [1] https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Andreas Christoforou <andreaschristofo@gmail.com> --- v2: - use XFRM_MAX_DEPTH for "count" array (Steffen and Mathias). --- net/ipv6/xfrm6_state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)