Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493011 > unrolled thread

[PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking

Started byJarod Wilson <jarod@redhat.com>
First post2016-09-29 00:30 +0200
Last post2016-10-08 04:10 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking Jarod Wilson <jarod@redhat.com> - 2016-09-29 00:30 +0200
    Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking Jakub Sitnicki <jkbs@redhat.com> - 2016-09-30 11:40 +0200
      Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU  checking David Miller <davem@davemloft.net> - 2016-10-03 04:50 +0200
        Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU  checking Jarod Wilson <jarod@redhat.com> - 2016-10-03 19:50 +0200
        [PATCH v3 net-next 0/2] net: centralize net_device MTU bounds checking Jarod Wilson <jarod@redhat.com> - 2016-10-08 04:10 +0200
          [PATCH v3 net-next 1/2] net: centralize net_device min/max MTU checking Jarod Wilson <jarod@redhat.com> - 2016-10-08 04:10 +0200

#1493011 — [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking

FromJarod Wilson <jarod@redhat.com>
Date2016-09-29 00:30 +0200
Subject[PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
Message-ID<smvwt-4hA-17@gated-at.bofh.it>
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" <davem@davemloft.net>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 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 int		mtu;
+	unsigned int		min_mtu;
+	unsigned int		max_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

[toc] | [next] | [standalone]

#1493988

FromJakub Sitnicki <jkbs@redhat.com>
Date2016-09-30 11:40 +0200
Message-ID<sn2sp-8pw-7@gated-at.bofh.it>
In reply to#1493011
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" <davem@davemloft.net>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---

[...]

> 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

[toc] | [prev] | [next] | [standalone]

#1494686 — Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking

FromDavid Miller <davem@davemloft.net>
Date2016-10-03 04:50 +0200
SubjectRe: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
Message-ID<so1uh-6FG-3@gated-at.bofh.it>
In reply to#1493988
From: Jakub Sitnicki <jkbs@redhat.com>
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" <davem@davemloft.net>
>> CC: netdev@vger.kernel.org
>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> ---
> 
> [...]
> 
>> 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.

[toc] | [prev] | [next] | [standalone]

#1494984 — Re: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking

FromJarod Wilson <jarod@redhat.com>
Date2016-10-03 19:50 +0200
SubjectRe: [PATCH v2 net-next 1/2] net: centralize net_device min/max MTU checking
Message-ID<sofxf-7Za-7@gated-at.bofh.it>
In reply to#1494686
On Sun, Oct 02, 2016 at 10:43:22PM -0400, David Miller wrote:
> From: Jakub Sitnicki <jkbs@redhat.com>
> 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" <davem@davemloft.net>
> >> CC: netdev@vger.kernel.org
> >> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> >> ---
> > 
> > [...]
> > 
> >> 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
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1497642 — [PATCH v3 net-next 0/2] net: centralize net_device MTU bounds checking

FromJarod Wilson <jarod@redhat.com>
Date2016-10-08 04:10 +0200
Subject[PATCH v3 net-next 0/2] net: centralize net_device MTU bounds checking
Message-ID<spPfj-LZ-7@gated-at.bofh.it>
In reply to#1494686
Jarod Wilson (2):
  net: centralize net_device min/max MTU checking
  net: deprecate eth_change_mtu, remove usage

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

This pair of patches looks to introduce centralized MTU range checking
infrastructure, while maintaining compatibility with all existing drivers,
and start to make use of it, converting all eth_change_mtu/ether_setup users
over to this new infra.

Assuming these pass review muster, I've got a ton of follow-on patches to
clean up MTU settings for everything in the kernel with an ndo_change_mtu.

This work is all staged in a (rebasing) git tree here:

https://github.com/jarodwilson/linux-muck

The master branch is based on net-next from Oct 7, and carries these two
patches, plus a ton of follow-on patches to eliminate MTU range checks
and change_mtu functions where possible. All patches were successfully
built across 160 various arch and config combos by the 0-day folks.
(Thanks to Andrew Lunn for the suggestion to get that going).

Jarod Wilson (2):
  net: centralize net_device min/max MTU checking
  net: deprecate eth_change_mtu, remove usage

CC: netdev@vger.kernel.org
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

#1497643 — [PATCH v3 net-next 1/2] net: centralize net_device min/max MTU checking

FromJarod Wilson <jarod@redhat.com>
Date2016-10-08 04:10 +0200
Subject[PATCH v3 net-next 1/2] net: centralize net_device min/max MTU checking
Message-ID<spPfj-LZ-15@gated-at.bofh.it>
In reply to#1497642
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: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
v3: retain new_mtu < 0 check to prevent integral promotions,
    as pointed out by Jakub Sitnicki.

 include/linux/netdevice.h |  4 ++++
 net/core/dev.c            | 13 +++++++++++--
 2 files changed, 15 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 int		mtu;
+	unsigned int		min_mtu;
+	unsigned int		max_mtu;
 	unsigned short		type;
 	unsigned short		hard_header_len;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index f1fe26f..f376639 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6499,9 +6499,18 @@ 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)
+	/* MTU must be positive, and in range */
+	if (new_mtu < 0 || 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web