| Message ID | 1532011832-6952-1-git-send-email-tariqt@mellanox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: remove redundant input checks in SIOCSIFTXQLEN case of dev_ifsioc | expand |
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.
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?
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.
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.
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. >
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';
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(-)