| Message ID | 20200603081124.1627600-1-matthieu.baerts@tessares.net |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: fix unused-var without NETDEVICES | expand |
Matthieu Baerts <matthieu.baerts@tessares.net> ezt írta (időpont: 2020. jún. 3., Sze, 10:11): > > A recent commit added new variables only used if CONFIG_NETDEVICES is > set. Thank you for noticing and fixed this! > A simple fix is to only declare these variables if the same > condition is valid. > > Other solutions could be to move the code related to SO_BINDTODEVICE > option from _bpf_setsockopt() function to a dedicated one or only > declare these variables in the related "case" section. Yes thats indeed a cleaner way to approach this. I will prepare a fix for that. > > Fixes: 70c58997c1e8 ("bpf: Allow SO_BINDTODEVICE opt in bpf_setsockopt") > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> > --- > > Notes: > This fix currently applies on net-next and bpf-next only. Except that > net-next is now closed and -net will get commits from net-next after > Linus' pull. > > I hope it is fine to have picked [PATCH bpf] and not bpf-next (or net). > > net/core/filter.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index d01a244b5087..ee08c6fcee1a 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -4286,9 +4286,11 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = { > static int _bpf_setsockopt(struct sock *sk, int level, int optname, > char *optval, int optlen, u32 flags) > { > +#ifdef CONFIG_NETDEVICES > char devname[IFNAMSIZ]; > struct net *net; > int ifindex; > +#endif > int ret = 0; > int val; > > -- > 2.25.1 >
Hi Ferenc, On 03/06/2020 10:56, Ferenc Fejes wrote: > Matthieu Baerts <matthieu.baerts@tessares.net> ezt írta (időpont: > 2020. jún. 3., Sze, 10:11): >> >> A recent commit added new variables only used if CONFIG_NETDEVICES is >> set. > > Thank you for noticing and fixed this! > >> A simple fix is to only declare these variables if the same >> condition is valid. >> >> Other solutions could be to move the code related to SO_BINDTODEVICE >> option from _bpf_setsockopt() function to a dedicated one or only >> declare these variables in the related "case" section. > > Yes thats indeed a cleaner way to approach this. I will prepare a fix for that. I should have maybe added that I didn't take this approach because in the rest of the code, I don't see that variables are declared only in a "case" section (no "{" ... "}" after "case") and code is generally not moved into a dedicated function in these big switch/cases. But maybe it makes sense here because of the #ifdef! At the end, I took the simple approach because it is for -net. In other words, I don't know what maintainers would prefer here but I am happy to see any another solutions implemented to remove these compiler warnings :) Cheers, Matt
On Wed, Jun 03, 2020 at 11:12:01AM +0200, Matthieu Baerts wrote: > Hi Ferenc, > > On 03/06/2020 10:56, Ferenc Fejes wrote: > > Matthieu Baerts <matthieu.baerts@tessares.net> ezt írta (időpont: > > 2020. jún. 3., Sze, 10:11): > > > > > > A recent commit added new variables only used if CONFIG_NETDEVICES is > > > set. > > > > Thank you for noticing and fixed this! > > > > > A simple fix is to only declare these variables if the same > > > condition is valid. > > > > > > Other solutions could be to move the code related to SO_BINDTODEVICE > > > option from _bpf_setsockopt() function to a dedicated one or only > > > declare these variables in the related "case" section. > > > > Yes thats indeed a cleaner way to approach this. I will prepare a fix for that. > > I should have maybe added that I didn't take this approach because in the > rest of the code, I don't see that variables are declared only in a "case" > section (no "{" ... "}" after "case") and code is generally not moved into a > dedicated function in these big switch/cases. But maybe it makes sense here > because of the #ifdef! > At the end, I took the simple approach because it is for -net. > > In other words, I don't know what maintainers would prefer here but I am > happy to see any another solutions implemented to remove these compiler > warnings :) since CONFIG_NETDEVICES doesn't change anything in .h I think the best is to remove #ifdef CONFIG_NETDEVICES from net/core/filter.c and rely on sock_bindtoindex() returning ENOPROTOOPT in the extreme case of oddly configured kernels.
Hi Alexei, On 03/06/2020 20:14, Alexei Starovoitov wrote: > On Wed, Jun 03, 2020 at 11:12:01AM +0200, Matthieu Baerts wrote: >> Hi Ferenc, >> >> On 03/06/2020 10:56, Ferenc Fejes wrote: >>> Matthieu Baerts <matthieu.baerts@tessares.net> ezt írta (időpont: >>> 2020. jún. 3., Sze, 10:11): >>>> >>>> A recent commit added new variables only used if CONFIG_NETDEVICES is >>>> set. >>> >>> Thank you for noticing and fixed this! >>> >>>> A simple fix is to only declare these variables if the same >>>> condition is valid. >>>> >>>> Other solutions could be to move the code related to SO_BINDTODEVICE >>>> option from _bpf_setsockopt() function to a dedicated one or only >>>> declare these variables in the related "case" section. >>> >>> Yes thats indeed a cleaner way to approach this. I will prepare a fix for that. >> >> I should have maybe added that I didn't take this approach because in the >> rest of the code, I don't see that variables are declared only in a "case" >> section (no "{" ... "}" after "case") and code is generally not moved into a >> dedicated function in these big switch/cases. But maybe it makes sense here >> because of the #ifdef! >> At the end, I took the simple approach because it is for -net. >> >> In other words, I don't know what maintainers would prefer here but I am >> happy to see any another solutions implemented to remove these compiler >> warnings :) > > since CONFIG_NETDEVICES doesn't change anything in .h > I think the best is to remove #ifdef CONFIG_NETDEVICES from net/core/filter.c > and rely on sock_bindtoindex() returning ENOPROTOOPT > in the extreme case of oddly configured kernels. Good idea, thank you! I can send a patch implementing that. And sorry for the oddly configured kernels :) It's just used to test the compilation of the code related to MPTCP. Cheers, Matt
On Wed, Jun 3, 2020 at 11:41 AM Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > > Hi Alexei, > > On 03/06/2020 20:14, Alexei Starovoitov wrote: > > On Wed, Jun 03, 2020 at 11:12:01AM +0200, Matthieu Baerts wrote: > >> Hi Ferenc, > >> > >> On 03/06/2020 10:56, Ferenc Fejes wrote: > >>> Matthieu Baerts <matthieu.baerts@tessares.net> ezt írta (időpont: > >>> 2020. jún. 3., Sze, 10:11): > >>>> > >>>> A recent commit added new variables only used if CONFIG_NETDEVICES is > >>>> set. > >>> > >>> Thank you for noticing and fixed this! > >>> > >>>> A simple fix is to only declare these variables if the same > >>>> condition is valid. > >>>> > >>>> Other solutions could be to move the code related to SO_BINDTODEVICE > >>>> option from _bpf_setsockopt() function to a dedicated one or only > >>>> declare these variables in the related "case" section. > >>> > >>> Yes thats indeed a cleaner way to approach this. I will prepare a fix for that. > >> > >> I should have maybe added that I didn't take this approach because in the > >> rest of the code, I don't see that variables are declared only in a "case" > >> section (no "{" ... "}" after "case") and code is generally not moved into a > >> dedicated function in these big switch/cases. But maybe it makes sense here > >> because of the #ifdef! > >> At the end, I took the simple approach because it is for -net. > >> > >> In other words, I don't know what maintainers would prefer here but I am > >> happy to see any another solutions implemented to remove these compiler > >> warnings :) > > > > since CONFIG_NETDEVICES doesn't change anything in .h > > I think the best is to remove #ifdef CONFIG_NETDEVICES from net/core/filter.c > > and rely on sock_bindtoindex() returning ENOPROTOOPT > > in the extreme case of oddly configured kernels. > > Good idea, thank you! > I can send a patch implementing that. Please do. Your 'Notes:' section was absolutely correct in terms of different trees relationship :) Thank you.
> > Hi Alexei, > > On 03/06/2020 20:14, Alexei Starovoitov wrote: > > On Wed, Jun 03, 2020 at 11:12:01AM +0200, Matthieu Baerts wrote: > >> Hi Ferenc, > >> > >> On 03/06/2020 10:56, Ferenc Fejes wrote: > >>> Matthieu Baerts <matthieu.baerts@tessares.net> ezt írta (időpont: > >>> 2020. jún. 3., Sze, 10:11): > >>>> > >>>> A recent commit added new variables only used if CONFIG_NETDEVICES is > >>>> set. > >>> > >>> Thank you for noticing and fixed this! > >>> > >>>> A simple fix is to only declare these variables if the same > >>>> condition is valid. > >>>> > >>>> Other solutions could be to move the code related to SO_BINDTODEVICE > >>>> option from _bpf_setsockopt() function to a dedicated one or only > >>>> declare these variables in the related "case" section. > >>> > >>> Yes thats indeed a cleaner way to approach this. I will prepare a fix for that. > >> > >> I should have maybe added that I didn't take this approach because in the > >> rest of the code, I don't see that variables are declared only in a "case" > >> section (no "{" ... "}" after "case") and code is generally not moved into a > >> dedicated function in these big switch/cases. But maybe it makes sense here > >> because of the #ifdef! > >> At the end, I took the simple approach because it is for -net. > >> > >> In other words, I don't know what maintainers would prefer here but I am > >> happy to see any another solutions implemented to remove these compiler > >> warnings :) > > > > since CONFIG_NETDEVICES doesn't change anything in .h > > I think the best is to remove #ifdef CONFIG_NETDEVICES from net/core/filter.c > > and rely on sock_bindtoindex() returning ENOPROTOOPT > > in the extreme case of oddly configured kernels. > > Good idea, thank you! > I can send a patch implementing that. Thanks again for helping me out in this one, I'll be more careful next time! > > And sorry for the oddly configured kernels :) > It's just used to test the compilation of the code related to MPTCP. > > Cheers, > Matt > -- > Matthieu Baerts | R&D Engineer > matthieu.baerts@tessares.net > Tessares SA | Hybrid Access Solutions > www.tessares.net > 1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
diff --git a/net/core/filter.c b/net/core/filter.c index d01a244b5087..ee08c6fcee1a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -4286,9 +4286,11 @@ static const struct bpf_func_proto bpf_get_socket_uid_proto = { static int _bpf_setsockopt(struct sock *sk, int level, int optname, char *optval, int optlen, u32 flags) { +#ifdef CONFIG_NETDEVICES char devname[IFNAMSIZ]; struct net *net; int ifindex; +#endif int ret = 0; int val;
A recent commit added new variables only used if CONFIG_NETDEVICES is set. A simple fix is to only declare these variables if the same condition is valid. Other solutions could be to move the code related to SO_BINDTODEVICE option from _bpf_setsockopt() function to a dedicated one or only declare these variables in the related "case" section. Fixes: 70c58997c1e8 ("bpf: Allow SO_BINDTODEVICE opt in bpf_setsockopt") Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> --- Notes: This fix currently applies on net-next and bpf-next only. Except that net-next is now closed and -net will get commits from net-next after Linus' pull. I hope it is fine to have picked [PATCH bpf] and not bpf-next (or net). net/core/filter.c | 2 ++ 1 file changed, 2 insertions(+)