[PATCH bpf] bpf: fix unused-var without NETDEVICES

[PATCH bpf] bpf: fix unused-var without NETDEVICES

[Matthieu Baerts]

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(+)

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

Re: [PATCH bpf] bpf: fix unused-var without NETDEVICES

[Ferenc Fejes]

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
>

Re: [PATCH bpf] bpf: fix unused-var without NETDEVICES

[Matthieu Baerts]

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
-- 
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

Re: [PATCH bpf] bpf: fix unused-var without NETDEVICES

[Alexei Starovoitov]

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.

Re: [PATCH bpf] bpf: fix unused-var without NETDEVICES

[Matthieu Baerts]

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
-- 
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

Re: [PATCH bpf] bpf: fix unused-var without NETDEVICES

[Alexei Starovoitov]

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.

[PATCH bpf v2] bpf: fix unused-var without NETDEVICES

[Matthieu Baerts]

A recent commit added new variables only used if CONFIG_NETDEVICES is
set. A simple fix would be to only declare these variables if the same
condition is valid but Alexei suggested an even simpler solution:

    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.

Fixes: 70c58997c1e8 ("bpf: Allow SO_BINDTODEVICE opt in bpf_setsockopt")
Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
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.

    v2: remove #ifdef CONFIG_NETDEVICES (Alexei)

 net/core/filter.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index d01a244b5087..90d2eb77002f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4340,8 +4340,6 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
 			}
 			break;
 		case SO_BINDTODEVICE:
-			ret = -ENOPROTOOPT;
-#ifdef CONFIG_NETDEVICES
 			optlen = min_t(long, optlen, IFNAMSIZ - 1);
 			strncpy(devname, optval, optlen);
 			devname[optlen] = 0;
@@ -4360,7 +4358,6 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname,
 				dev_put(dev);
 			}
 			ret = sock_bindtoindex(sk, ifindex, false);
-#endif
 			break;
 		default:
 			ret = -EINVAL;
-- 
2.25.1

Re: [PATCH bpf v2] bpf: fix unused-var without NETDEVICES

[Song Liu]

On Wed, Jun 3, 2020 at 12:05 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> A recent commit added new variables only used if CONFIG_NETDEVICES is
> set. A simple fix would be to only declare these variables if the same
> condition is valid but Alexei suggested an even simpler solution:
>
>     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.
>
> Fixes: 70c58997c1e8 ("bpf: Allow SO_BINDTODEVICE opt in bpf_setsockopt")
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH bpf v2] bpf: fix unused-var without NETDEVICES

[Daniel Borkmann]

On 6/3/20 10:45 PM, Song Liu wrote:
> On Wed, Jun 3, 2020 at 12:05 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> A recent commit added new variables only used if CONFIG_NETDEVICES is
>> set. A simple fix would be to only declare these variables if the same
>> condition is valid but Alexei suggested an even simpler solution:
>>
>>      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.
>>
>> Fixes: 70c58997c1e8 ("bpf: Allow SO_BINDTODEVICE opt in bpf_setsockopt")
>> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> Acked-by: Song Liu <songliubraving@fb.com>

Applied, thanks!

Re: [PATCH bpf] bpf: fix unused-var without NETDEVICES

[Ferenc Fejes]

>
> 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