* [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc
@ 2018-07-19 14:50 Tariq Toukan
2018-07-19 17:21 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Tariq Toukan @ 2018-07-19 14:50 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Eran Ben Elisha, Tariq Toukan, Cong Wang
The cited patch added a call to dev_change_tx_queue_len in
SIOCSIFTXQLEN case.
This obsoletes the checks done before the function call.
Remove them here.
Fixes: 3f76df198288 ("net: use dev_change_tx_queue_len() for SIOCSIFTXQLEN")
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
---
net/core/dev_ioctl.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 50537ff961a7..7c1ad40322a3 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -282,14 +282,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
case SIOCSIFTXQLEN:
- if (ifr->ifr_qlen < 0)
- return -EINVAL;
- if (dev->tx_queue_len ^ ifr->ifr_qlen) {
- err = dev_change_tx_queue_len(dev, ifr->ifr_qlen);
- if (err)
- return err;
- }
- return 0;
+ return dev_change_tx_queue_len(dev, ifr->ifr_qlen);
case SIOCSIFNAME:
ifr->ifr_newname[IFNAMSIZ-1] = '\0';
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc
2018-07-19 14:50 [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc Tariq Toukan
@ 2018-07-19 17:21 ` Cong Wang
2018-07-22 7:29 ` Tariq Toukan
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2018-07-19 17:21 UTC (permalink / raw)
To: Tariq Toukan; +Cc: David Miller, Linux Kernel Network Developers, eranbe
On Thu, Jul 19, 2018 at 7:50 AM Tariq Toukan <tariqt@mellanox.com> wrote:
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -282,14 +282,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
> return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
>
> case SIOCSIFTXQLEN:
> - if (ifr->ifr_qlen < 0)
> - return -EINVAL;
Are you sure we can remove this if check too?
The other one is safe to remove.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc
2018-07-19 17:21 ` Cong Wang
@ 2018-07-22 7:29 ` Tariq Toukan
2018-07-23 20:37 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Tariq Toukan @ 2018-07-22 7:29 UTC (permalink / raw)
To: Cong Wang, Tariq Toukan
Cc: David Miller, Linux Kernel Network Developers, eranbe
On 19/07/2018 8:21 PM, Cong Wang wrote:
> On Thu, Jul 19, 2018 at 7:50 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>> --- a/net/core/dev_ioctl.c
>> +++ b/net/core/dev_ioctl.c
>> @@ -282,14 +282,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
>> return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
>>
>> case SIOCSIFTXQLEN:
>> - if (ifr->ifr_qlen < 0)
>> - return -EINVAL;
>
> Are you sure we can remove this if check too?
>
> The other one is safe to remove.
>
Hmm, let's see:
dev_change_tx_queue_len gets unsigned long new_len, any negative value
passed is interpreted as a very large number, then we test:
if (new_len != (unsigned int)new_len)
This test returns true if range of unsigned long is larger than range of
unsigned int. AFAIK these ranges are Arch dependent and there is no
guarantee this holds.
Right?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc
2018-07-22 7:29 ` Tariq Toukan
@ 2018-07-23 20:37 ` Cong Wang
2018-07-23 21:00 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2018-07-23 20:37 UTC (permalink / raw)
To: Tariq Toukan; +Cc: David Miller, Linux Kernel Network Developers, eranbe
On Sun, Jul 22, 2018 at 12:29 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>
>
>
> On 19/07/2018 8:21 PM, Cong Wang wrote:
> > On Thu, Jul 19, 2018 at 7:50 AM Tariq Toukan <tariqt@mellanox.com> wrote:
> >> --- a/net/core/dev_ioctl.c
> >> +++ b/net/core/dev_ioctl.c
> >> @@ -282,14 +282,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
> >> return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
> >>
> >> case SIOCSIFTXQLEN:
> >> - if (ifr->ifr_qlen < 0)
> >> - return -EINVAL;
> >
> > Are you sure we can remove this if check too?
> >
> > The other one is safe to remove.
> >
>
> Hmm, let's see:
> dev_change_tx_queue_len gets unsigned long new_len, any negative value
> passed is interpreted as a very large number, then we test:
> if (new_len != (unsigned int)new_len)
>
> This test returns true if range of unsigned long is larger than range of
> unsigned int. AFAIK these ranges are Arch dependent and there is no
> guarantee this holds.
>
I am not sure either, you probably have to give it a test.
And at least, explain it in changelog if you still want to remove it.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc
2018-07-23 20:37 ` Cong Wang
@ 2018-07-23 21:00 ` David Miller
2018-07-24 8:35 ` Tariq Toukan
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2018-07-23 21:00 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: tariqt, netdev, eranbe
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 23 Jul 2018 13:37:22 -0700
> On Sun, Jul 22, 2018 at 12:29 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>>
>>
>>
>> On 19/07/2018 8:21 PM, Cong Wang wrote:
>> > On Thu, Jul 19, 2018 at 7:50 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>> >> --- a/net/core/dev_ioctl.c
>> >> +++ b/net/core/dev_ioctl.c
>> >> @@ -282,14 +282,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
>> >> return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
>> >>
>> >> case SIOCSIFTXQLEN:
>> >> - if (ifr->ifr_qlen < 0)
>> >> - return -EINVAL;
>> >
>> > Are you sure we can remove this if check too?
>> >
>> > The other one is safe to remove.
>> >
>>
>> Hmm, let's see:
>> dev_change_tx_queue_len gets unsigned long new_len, any negative value
>> passed is interpreted as a very large number, then we test:
>> if (new_len != (unsigned int)new_len)
>>
>> This test returns true if range of unsigned long is larger than range of
>> unsigned int. AFAIK these ranges are Arch dependent and there is no
>> guarantee this holds.
>
> I am not sure either, you probably have to give it a test.
> And at least, explain it in changelog if you still want to remove it.
On 64-bit we will fail with -ERANGE. The 32-bit int ifr_qlen will be sign
extended to 64-bits when it is passed into dev_change_tx_queue_len(). And
then for negative values this test triggers:
if (new_len != (unsigned int)new_len)
return -ERANGE;
because:
if (0xffffffffWHATEVER != 0x00000000WHATEVER)
On 32-bit the signed value will be accepted, changing behavior.
I think, therefore, that the < 0 check should be retained.
Thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc
2018-07-23 21:00 ` David Miller
@ 2018-07-24 8:35 ` Tariq Toukan
0 siblings, 0 replies; 6+ messages in thread
From: Tariq Toukan @ 2018-07-24 8:35 UTC (permalink / raw)
To: David Miller, xiyou.wangcong; +Cc: tariqt, netdev, eranbe
On 24/07/2018 12:00 AM, David Miller wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 23 Jul 2018 13:37:22 -0700
>
>> On Sun, Jul 22, 2018 at 12:29 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>>>
>>>
>>>
>>> On 19/07/2018 8:21 PM, Cong Wang wrote:
>>>> On Thu, Jul 19, 2018 at 7:50 AM Tariq Toukan <tariqt@mellanox.com> wrote:
>>>>> --- a/net/core/dev_ioctl.c
>>>>> +++ b/net/core/dev_ioctl.c
>>>>> @@ -282,14 +282,7 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
>>>>> return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
>>>>>
>>>>> case SIOCSIFTXQLEN:
>>>>> - if (ifr->ifr_qlen < 0)
>>>>> - return -EINVAL;
>>>>
>>>> Are you sure we can remove this if check too?
>>>>
>>>> The other one is safe to remove.
>>>>
>>>
>>> Hmm, let's see:
>>> dev_change_tx_queue_len gets unsigned long new_len, any negative value
>>> passed is interpreted as a very large number, then we test:
>>> if (new_len != (unsigned int)new_len)
>>>
>>> This test returns true if range of unsigned long is larger than range of
>>> unsigned int. AFAIK these ranges are Arch dependent and there is no
>>> guarantee this holds.
>>
>> I am not sure either, you probably have to give it a test.
>> And at least, explain it in changelog if you still want to remove it.
>
> On 64-bit we will fail with -ERANGE. The 32-bit int ifr_qlen will be sign
> extended to 64-bits when it is passed into dev_change_tx_queue_len(). And
> then for negative values this test triggers:
>
> if (new_len != (unsigned int)new_len)
> return -ERANGE;
>
> because:
> if (0xffffffffWHATEVER != 0x00000000WHATEVER)
>
> On 32-bit the signed value will be accepted, changing behavior.
>
> I think, therefore, that the < 0 check should be retained.
Agree.
I am sending a re-spin.
>
> Thank you.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-24 9:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-19 14:50 [PATCH net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc Tariq Toukan
2018-07-19 17:21 ` Cong Wang
2018-07-22 7:29 ` Tariq Toukan
2018-07-23 20:37 ` Cong Wang
2018-07-23 21:00 ` David Miller
2018-07-24 8:35 ` Tariq Toukan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).