Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
On Sun, Oct 02, 2016 at 10:43:22PM -0400, David Miller wrote:
> From: Jakub Sitnicki
> Date: Fri, 30 Sep 2016 11:37:24 +0200
>
> > On Wed, Sep 28, 2016 at 10:20 PM GMT, Jarod Wilson wrote:
> >> While looking into an MTU issue with sfc, I started noticing that almost
> >> every NIC driver with an ndo_change_mtu function implemented almost
> >> exactly the same range checks, and in many cases, that was the only
> >> practical thing their ndo_change_mtu function was doing. Quite a few
> >> drivers have either 68, 64, 60 or 46 as their minimum MTU value checked,
> >> and then various sizes from 1500 to 65535 for their maximum MTU value. We
> >> can remove a whole lot of redundant code here if we simple store min_mtu
> >> and max_mtu in net_device, and check against those in net/core/dev.c's
> >> dev_set_mtu().
> >>
> >> In theory, there should be zero functional change with this patch, it just
> >> puts the infrastructure in place. Subsequent patches will attempt to start
> >> using said infrastructure, with theoretically zero change in
> >> functionality.
> >>
> >> CC: "David S. Miller"
> >> CC: netdev@vger.kernel.org
> >> Signed-off-by: Jarod Wilson
> >> ---
> >
> > [...]
> >
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index c0c291f..5343799 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -6493,9 +6493,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
> >>if (new_mtu == dev->mtu)
> >>return 0;
> >>
> >> - /* MTU must be positive.*/
> >> - if (new_mtu < 0)
> >> + if (new_mtu < dev->min_mtu) {
> >
> > Ouch, integral promotions. Looks like you need to keep the < 0 check.
> > Otherwise new_mtu gets promoted to unsigned int and negative values will
> > pass the check.
>
> Agreed, the < 0 test must be reintroduced.
Gah, yeah, okay, will add it back in. Thinking like this:
if (new_mtu < 0 || new_mtu < dev->min_mtu) {
Alternatively, could have the negative value check on it's own, with a
harsher warning about negative values.
--
Jarod Wilson
ja...@redhat.com
Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
From: Jakub Sitnicki
Date: Fri, 30 Sep 2016 11:37:24 +0200
> On Wed, Sep 28, 2016 at 10:20 PM GMT, Jarod Wilson wrote:
>> While looking into an MTU issue with sfc, I started noticing that almost
>> every NIC driver with an ndo_change_mtu function implemented almost
>> exactly the same range checks, and in many cases, that was the only
>> practical thing their ndo_change_mtu function was doing. Quite a few
>> drivers have either 68, 64, 60 or 46 as their minimum MTU value checked,
>> and then various sizes from 1500 to 65535 for their maximum MTU value. We
>> can remove a whole lot of redundant code here if we simple store min_mtu
>> and max_mtu in net_device, and check against those in net/core/dev.c's
>> dev_set_mtu().
>>
>> In theory, there should be zero functional change with this patch, it just
>> puts the infrastructure in place. Subsequent patches will attempt to start
>> using said infrastructure, with theoretically zero change in
>> functionality.
>>
>> CC: "David S. Miller"
>> CC: netdev@vger.kernel.org
>> Signed-off-by: Jarod Wilson
>> ---
>
> [...]
>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index c0c291f..5343799 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6493,9 +6493,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
>> if (new_mtu == dev->mtu)
>> return 0;
>>
>> -/* MTU must be positive.*/
>> -if (new_mtu < 0)
>> +if (new_mtu < dev->min_mtu) {
>
> Ouch, integral promotions. Looks like you need to keep the < 0 check.
> Otherwise new_mtu gets promoted to unsigned int and negative values will
> pass the check.
Agreed, the < 0 test must be reintroduced.
Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
On Wed, Sep 28, 2016 at 10:20 PM GMT, Jarod Wilson wrote:
> While looking into an MTU issue with sfc, I started noticing that almost
> every NIC driver with an ndo_change_mtu function implemented almost
> exactly the same range checks, and in many cases, that was the only
> practical thing their ndo_change_mtu function was doing. Quite a few
> drivers have either 68, 64, 60 or 46 as their minimum MTU value checked,
> and then various sizes from 1500 to 65535 for their maximum MTU value. We
> can remove a whole lot of redundant code here if we simple store min_mtu
> and max_mtu in net_device, and check against those in net/core/dev.c's
> dev_set_mtu().
>
> In theory, there should be zero functional change with this patch, it just
> puts the infrastructure in place. Subsequent patches will attempt to start
> using said infrastructure, with theoretically zero change in
> functionality.
>
> CC: "David S. Miller"
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson
> ---
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c0c291f..5343799 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6493,9 +6493,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
> if (new_mtu == dev->mtu)
> return 0;
>
> - /* MTU must be positive.*/
> - if (new_mtu < 0)
> + if (new_mtu < dev->min_mtu) {
Ouch, integral promotions. Looks like you need to keep the < 0 check.
Otherwise new_mtu gets promoted to unsigned int and negative values will
pass the check.
Thanks,
Jakub
[PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
While looking into an MTU issue with sfc, I started noticing that almost
every NIC driver with an ndo_change_mtu function implemented almost
exactly the same range checks, and in many cases, that was the only
practical thing their ndo_change_mtu function was doing. Quite a few
drivers have either 68, 64, 60 or 46 as their minimum MTU value checked,
and then various sizes from 1500 to 65535 for their maximum MTU value. We
can remove a whole lot of redundant code here if we simple store min_mtu
and max_mtu in net_device, and check against those in net/core/dev.c's
dev_set_mtu().
In theory, there should be zero functional change with this patch, it just
puts the infrastructure in place. Subsequent patches will attempt to start
using said infrastructure, with theoretically zero change in
functionality.
CC: "David S. Miller"
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson
---
include/linux/netdevice.h | 4
net/core/dev.c| 12 ++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 136ae6bb..fbdf923 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1506,6 +1506,8 @@ enum netdev_priv_flags {
* @if_port: Selectable AUI, TP, ...
* @dma: DMA channel
* @mtu: Interface MTU value
+ * @min_mtu: Interface Minimum MTU value
+ * @max_mtu: Interface Maximum MTU value
* @type: Interface hardware type
* @hard_header_len: Maximum hardware header length.
*
@@ -1726,6 +1728,8 @@ struct net_device {
unsigned char dma;
unsigned intmtu;
+ unsigned intmin_mtu;
+ unsigned intmax_mtu;
unsigned short type;
unsigned short hard_header_len;
diff --git a/net/core/dev.c b/net/core/dev.c
index c0c291f..5343799 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6493,9 +6493,17 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
if (new_mtu == dev->mtu)
return 0;
- /* MTU must be positive.*/
- if (new_mtu < 0)
+ if (new_mtu < dev->min_mtu) {
+ net_err_ratelimited("%s: Invalid MTU %d requested, hw min %d\n",
+ dev->name, new_mtu, dev->min_mtu);
return -EINVAL;
+ }
+
+ if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) {
+ net_err_ratelimited("%s: Invalid MTU %d requested, hw max %d\n",
+ dev->name, new_mtu, dev->min_mtu);
+ return -EINVAL;
+ }
if (!netif_device_present(dev))
return -ENODEV;
--
2.10.0
