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 <[email protected]>
---
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
Matthieu Baerts <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]> 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
[email protected]
Tessares SA | Hybrid Access Solutions
http://www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
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 <[email protected]> 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 <[email protected]> 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
[email protected]
Tessares SA | Hybrid Access Solutions
http://www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
On Wed, Jun 3, 2020 at 11:41 AM Matthieu Baerts
<[email protected]> 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 <[email protected]> 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 <[email protected]> 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
> [email protected]
> Tessares SA | Hybrid Access Solutions
> http://www.tessares.net
> 1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium
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 <[email protected]>
Signed-off-by: Matthieu Baerts <[email protected]>
---
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
On Wed, Jun 3, 2020 at 12:05 PM Matthieu Baerts
<[email protected]> 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 <[email protected]>
> Signed-off-by: Matthieu Baerts <[email protected]>
Acked-by: Song Liu <[email protected]>
On 6/3/20 10:45 PM, Song Liu wrote:
> On Wed, Jun 3, 2020 at 12:05 PM Matthieu Baerts
> <[email protected]> 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 <[email protected]>
>> Signed-off-by: Matthieu Baerts <[email protected]>
>
> Acked-by: Song Liu <[email protected]>
Applied, thanks!