min/max mtu field in struct net_device

min/max mtu field in struct net_device

[Arend Van Spriel]

Just wondering. Does anyone have any ideas on how to backport the two
patches below. The struct net_device now holds mtu range which network
subsystem checks. For a number of drivers and mac80211 it means they no
longer have .ndo_change_mtu callback. My guess is that we need patches
in backport of some sort to tackle this.

Regards,
Arend

commit 61e84623ace35ce48975e8f90bbbac7557c43d61
Author: Jarod Wilson <jarod@redhat.com>
Date:   Fri Oct 7 22:04:33 2016 -0400

    net: centralize net_device min/max MTU checking

commit 9c22b4a34eddbaa5b5243c8cd27e31aa36e676e1
Author: Jarod Wilson <jarod@redhat.com>
Date:   Thu Oct 20 13:55:18 2016 -0400

    net: use core MTU range checking in wireless drivers
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: min/max mtu field in struct net_device

[Johannes Berg]

On Mon, 2016-11-14 at 09:49 +0100, Arend Van Spriel wrote:
> Just wondering. Does anyone have any ideas on how to backport the two
> patches below. The struct net_device now holds mtu range which
> network subsystem checks. For a number of drivers and mac80211 it
> means they no longer have .ndo_change_mtu callback. My guess is that
> we need patches in backport of some sort to tackle this.

Yeah, I kinda saw this coming. Since in almost all cases using the new
min/max the ndo_change_mtu was actually removed, perhaps we can come up
with a way to spatch it back in?

johannes
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: min/max mtu field in struct net_device

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On 14-11-2016 10:30, Johannes Berg wrote:
> On Mon, 2016-11-14 at 09:49 +0100, Arend Van Spriel wrote:
>> Just wondering. Does anyone have any ideas on how to backport the two
>> patches below. The struct net_device now holds mtu range which
>> network subsystem checks. For a number of drivers and mac80211 it
>> means they no longer have .ndo_change_mtu callback. My guess is that
>> we need patches in backport of some sort to tackle this.
> 
> Yeah, I kinda saw this coming. Since in almost all cases using the new
> min/max the ndo_change_mtu was actually removed, perhaps we can come up
> with a way to spatch it back in?

Me too. This is what I got so far, but the callback is generated twice
for net/mac80211/iface.c for obvious reasons (data and monitor ops).
These are my first baby steps with spatch so not sure how to solve it.

Gr. AvS

[-- Attachment #2: 0073-netdevice-mtu-range.cocci --]
[-- Type: text/plain, Size: 395 bytes --]

@r1@
expression ndev, e1, e2;
identifier func;
@@
func(...) {
	<...
- 	ndev->min_mtu = e1;
- 	ndev->max_mtu = e2;
	...>
}
@@
expression r1.e1, r1.e2;
identifier OPS;
@@
+ static int __change_mtu(struct net_device *ndev, int new_mtu)
+ {
+	if (new_mtu < e1 || new_mtu > e2)
+ 		return -EINVAL;
+	ndev->mtu = new_mtu;
+ }
+
struct net_device_ops OPS = {
+ 	.ndo_change_mtu = __change_mtu,
	...
};

Re: min/max mtu field in struct net_device

[Johannes Berg]

> Me too. This is what I got so far, but the callback is generated
> twice
> for net/mac80211/iface.c for obvious reasons (data and monitor ops).
> These are my first baby steps with spatch so not sure how to solve
> it.

Oh, right, that's problematic - no easy way to link the two and know
which one is which ...

I guess we'll have to carry that as a manual patch. We should probably
also ifdef on the kernel version.

johannes
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Fwd: Re: min/max mtu field in struct net_device

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

Hi Julia,

Would you have any ideas on how to solve the problem below. The problem
occurs with source files that have more than one struct net_device_ops
defined, which is the case for net/mac80211/iface.c for one. Obviously I
want the callback definition to be added only once.

Regards,
Arend

-------- Forwarded Message --------
Subject: Re: min/max mtu field in struct net_device
Date: Mon, 14 Nov 2016 11:46:01 +0100
From: Arend Van Spriel <arend.vanspriel@broadcom.com>
To: Johannes Berg <johannes@sipsolutions.net>, backports@vger.kernel.org
<backports@vger.kernel.org>

On 14-11-2016 10:30, Johannes Berg wrote:
> On Mon, 2016-11-14 at 09:49 +0100, Arend Van Spriel wrote:
>> Just wondering. Does anyone have any ideas on how to backport the two
>> patches below. The struct net_device now holds mtu range which
>> network subsystem checks. For a number of drivers and mac80211 it
>> means they no longer have .ndo_change_mtu callback. My guess is that
>> we need patches in backport of some sort to tackle this.
> 
> Yeah, I kinda saw this coming. Since in almost all cases using the new
> min/max the ndo_change_mtu was actually removed, perhaps we can come up
> with a way to spatch it back in?

Me too. This is what I got so far, but the callback is generated twice
for net/mac80211/iface.c for obvious reasons (data and monitor ops).
These are my first baby steps with spatch so not sure how to solve it.

Gr. AvS


[-- Attachment #2: 0073-netdevice-mtu-range.cocci --]
[-- Type: text/plain, Size: 395 bytes --]

@r1@
expression ndev, e1, e2;
identifier func;
@@
func(...) {
	<...
- 	ndev->min_mtu = e1;
- 	ndev->max_mtu = e2;
	...>
}
@@
expression r1.e1, r1.e2;
identifier OPS;
@@
+ static int __change_mtu(struct net_device *ndev, int new_mtu)
+ {
+	if (new_mtu < e1 || new_mtu > e2)
+ 		return -EINVAL;
+	ndev->mtu = new_mtu;
+ }
+
struct net_device_ops OPS = {
+ 	.ndo_change_mtu = __change_mtu,
	...
};

[Cocci] Fwd: Fwd: Re: min/max mtu field in struct net_device

[Arend Van Spriel]

Not seen a response on the backports mailing list so subscribed to the
cocci mailing list and reposting it.

Attached is a script I want to use to backport drivers. However, the
second (unnamed) rule matches for each occurence of 'struct
net_device_ops'. Is there a way to disable a rule after a desired amount
of matches (in my case once)?

Regards,
Arend

-------- Forwarded Message --------
Subject: Fwd: Re: min/max mtu field in struct net_device
Date: Tue, 15 Nov 2016 09:36:50 +0100
From: Arend Van Spriel <arend.vanspriel@broadcom.com>
To: Julia Lawall <Julia.Lawall@lip6.fr>
CC: backports at vger.kernel.org <backports@vger.kernel.org>

Hi Julia,

Would you have any ideas on how to solve the problem below. The problem
occurs with source files that have more than one struct net_device_ops
defined, which is the case for net/mac80211/iface.c for one. Obviously I
want the callback definition to be added only once.

Regards,
Arend

-------- Forwarded Message --------
Subject: Re: min/max mtu field in struct net_device
Date: Mon, 14 Nov 2016 11:46:01 +0100
From: Arend Van Spriel <arend.vanspriel@broadcom.com>
To: Johannes Berg <johannes@sipsolutions.net>, backports at vger.kernel.org
<backports@vger.kernel.org>

On 14-11-2016 10:30, Johannes Berg wrote:
> On Mon, 2016-11-14 at 09:49 +0100, Arend Van Spriel wrote:
>> Just wondering. Does anyone have any ideas on how to backport the two
>> patches below. The struct net_device now holds mtu range which
>> network subsystem checks. For a number of drivers and mac80211 it
>> means they no longer have .ndo_change_mtu callback. My guess is that
>> we need patches in backport of some sort to tackle this.
> 
> Yeah, I kinda saw this coming. Since in almost all cases using the new
> min/max the ndo_change_mtu was actually removed, perhaps we can come up
> with a way to spatch it back in?

Me too. This is what I got so far, but the callback is generated twice
for net/mac80211/iface.c for obvious reasons (data and monitor ops).
These are my first baby steps with spatch so not sure how to solve it.

Gr. AvS


-------------- next part --------------
@r1@
expression ndev, e1, e2;
identifier func;
@@
func(...) {
	<...
- 	ndev->min_mtu = e1;
- 	ndev->max_mtu = e2;
	...>
}
@@
expression r1.e1, r1.e2;
identifier OPS;
@@
+ static int __change_mtu(struct net_device *ndev, int new_mtu)
+ {
+	if (new_mtu < e1 || new_mtu > e2)
+ 		return -EINVAL;
+	ndev->mtu = new_mtu;
+ }
+
struct net_device_ops OPS = {
+ 	.ndo_change_mtu = __change_mtu,
	...
};

[Cocci] Fwd: Fwd: Re: min/max mtu field in struct net_device

[Julia Lawall]

On Thu, 17 Nov 2016, Arend Van Spriel wrote:

> Not seen a response on the backports mailing list so subscribed to the
> cocci mailing list and reposting it.

Sorry to have missed seeing it.

>
> Attached is a script I want to use to backport drivers. However, the
> second (unnamed) rule matches for each occurence of 'struct
> net_device_ops'. Is there a way to disable a rule after a desired amount
> of matches (in my case once)?

Here is a somewhat clunky solution:

@initialize:python@
@@

first_ops = 0

@r@
identifier OPS;
position p;
@@

struct net_device_ops OPS at p = { ... };

@script:python depends on r@
@@

first_ops = 0

@script:python@
p << r.p;
@@

ln = int(p[0].line)
if first_ops == 0 or ln < first_ops:
  first_ops = ln

@script:python@
p << r.p;
@@

ln = int(p[0].line)
if not(first_ops == ln):
  cocci.include_match(False)

@r1 exists@
expression ndevexp, e1, e2;
identifier func;
@@
func(...) {
          <+...
-         ndevexp->min_mtu = e1;
-         ndevexp->max_mtu = e2;
          ...+>
}

@r2@
expression r1.e1,r1.e2;
identifier r.OPS;
@@
+ static int __change_mtu(struct net_device *ndev, int new_mtu)
+ {
+ if (new_mtu < e1 || new_mtu > e2)
+             return -EINVAL;
+             ndev->mtu = new_mtu;
+ }
+
struct net_device_ops OPS = {
       ...
};

@depends on r2@
identifier OPS;
@@

struct net_device_ops OPS = {
+      .ndo_change_mtu = __change_mtu,
       ...
};

-------------------------

Basically, the idea is to find the line number of the start of each
net_device_ops structure.  The first script:python rule initializes
first_ops for the current file.  The second one passes through the
position information for each net_device_ops structure and finds the
smallest line number.  The third python rule discards all matches that
don't have the name of the structure on the collected smallest line.  Rule
r1 then matches the two assignments, rule r2 puts the new function on top
of the earliest detected structure (determined by the inherited value of
OPS), and then the last rule added the field to the structure.

I noticed that there is a problem though when the structure is define
within a function.  Then the added function goes in that structure too.
The affected file is: drivers/tty/n_gsm.c.  Maybe you odn't want o
backport that one?

julia

[Cocci] Fwd: Fwd: Re: min/max mtu field in struct net_device

[Arend van Spriel]

On 17-11-16 23:48, Julia Lawall wrote:
> 
> 
> On Thu, 17 Nov 2016, Arend Van Spriel wrote:
> 
>> Not seen a response on the backports mailing list so subscribed to the
>> cocci mailing list and reposting it.
> 
> Sorry to have missed seeing it.

No problem.

>>
>> Attached is a script I want to use to backport drivers. However, the
>> second (unnamed) rule matches for each occurence of 'struct
>> net_device_ops'. Is there a way to disable a rule after a desired amount
>> of matches (in my case once)?
> 
> Here is a somewhat clunky solution:
> 
> @initialize:python@
> @@
> 
> first_ops = 0
> 
> @r@
> identifier OPS;
> position p;
> @@
> 
> struct net_device_ops OPS at p = { ... };
> 
> @script:python depends on r@
> @@
> 
> first_ops = 0
> 
> @script:python@
> p << r.p;
> @@
> 
> ln = int(p[0].line)
> if first_ops == 0 or ln < first_ops:
>   first_ops = ln
> 
> @script:python@
> p << r.p;
> @@
> 
> ln = int(p[0].line)
> if not(first_ops == ln):
>   cocci.include_match(False)
> 
> @r1 exists@
> expression ndevexp, e1, e2;
> identifier func;
> @@
> func(...) {
>           <+...
> -         ndevexp->min_mtu = e1;
> -         ndevexp->max_mtu = e2;
>           ...+>
> }
> 
> @r2@
> expression r1.e1,r1.e2;
> identifier r.OPS;
> @@
> + static int __change_mtu(struct net_device *ndev, int new_mtu)
> + {
> + if (new_mtu < e1 || new_mtu > e2)
> +             return -EINVAL;
> +             ndev->mtu = new_mtu;
> + }
> +
> struct net_device_ops OPS = {
>        ...
> };
> 
> @depends on r2@
> identifier OPS;
> @@
> 
> struct net_device_ops OPS = {
> +      .ndo_change_mtu = __change_mtu,
>        ...
> };
> 
> -------------------------

Big thanks. Have not tried it yet, but do you want me to add your
Signed-off-by given that you mostly solved the problem.

> Basically, the idea is to find the line number of the start of each
> net_device_ops structure.  The first script:python rule initializes
> first_ops for the current file.  The second one passes through the
> position information for each net_device_ops structure and finds the
> smallest line number.  The third python rule discards all matches that
> don't have the name of the structure on the collected smallest line.

Yeah. Was wondering how to tackle that as I read in the documentation
that the order in which matches are processed is not predictable.

>  Rule
> r1 then matches the two assignments, rule r2 puts the new function on top
> of the earliest detected structure (determined by the inherited value of
> OPS), and then the last rule added the field to the structure.

Understand. Thanks again.

> I noticed that there is a problem though when the structure is define
> within a function.  Then the added function goes in that structure too.
> The affected file is: drivers/tty/n_gsm.c.  Maybe you odn't want o
> backport that one?

It is not (yet) in the backports copy-list so we are good for now.

Regards,
Arend

[Cocci] Fwd: Fwd: Re: min/max mtu field in struct net_device

[Julia Lawall]

> >> Attached is a script I want to use to backport drivers. However, the
> >> second (unnamed) rule matches for each occurence of 'struct
> >> net_device_ops'. Is there a way to disable a rule after a desired amount
> >> of matches (in my case once)?
> >
> > Here is a somewhat clunky solution:
> >
> > @initialize:python@
> > @@
> >
> > first_ops = 0
> >
> > @r@
> > identifier OPS;
> > position p;
> > @@
> >
> > struct net_device_ops OPS at p = { ... };
> >
> > @script:python depends on r@
> > @@
> >
> > first_ops = 0
> >
> > @script:python@
> > p << r.p;
> > @@
> >
> > ln = int(p[0].line)
> > if first_ops == 0 or ln < first_ops:
> >   first_ops = ln
> >
> > @script:python@
> > p << r.p;
> > @@
> >
> > ln = int(p[0].line)
> > if not(first_ops == ln):
> >   cocci.include_match(False)
> >
> > @r1 exists@
> > expression ndevexp, e1, e2;
> > identifier func;
> > @@
> > func(...) {
> >           <+...
> > -         ndevexp->min_mtu = e1;
> > -         ndevexp->max_mtu = e2;
> >           ...+>
> > }
> >
> > @r2@
> > expression r1.e1,r1.e2;
> > identifier r.OPS;
> > @@
> > + static int __change_mtu(struct net_device *ndev, int new_mtu)
> > + {
> > + if (new_mtu < e1 || new_mtu > e2)
> > +             return -EINVAL;
> > +             ndev->mtu = new_mtu;
> > + }
> > +
> > struct net_device_ops OPS = {
> >        ...
> > };
> >
> > @depends on r2@
> > identifier OPS;
> > @@
> >
> > struct net_device_ops OPS = {
> > +      .ndo_change_mtu = __change_mtu,
> >        ...
> > };
> >
> > -------------------------
>
> Big thanks. Have not tried it yet, but do you want me to add your
> Signed-off-by given that you mostly solved the problem.

OK.

>
> > Basically, the idea is to find the line number of the start of each
> > net_device_ops structure.  The first script:python rule initializes
> > first_ops for the current file.  The second one passes through the
> > position information for each net_device_ops structure and finds the
> > smallest line number.  The third python rule discards all matches that
> > don't have the name of the structure on the collected smallest line.
>
> Yeah. Was wondering how to tackle that as I read in the documentation
> that the order in which matches are processed is not predictable.
>
> >  Rule
> > r1 then matches the two assignments, rule r2 puts the new function on top
> > of the earliest detected structure (determined by the inherited value of
> > OPS), and then the last rule added the field to the structure.
>
> Understand. Thanks again.
>
> > I noticed that there is a problem though when the structure is define
> > within a function.  Then the added function goes in that structure too.
> > The affected file is: drivers/tty/n_gsm.c.  Maybe you odn't want o
> > backport that one?
>
> It is not (yet) in the backports copy-list so we are good for now.

Great.  I guess one could solve the issue by more clunky position
manipulations to detect when a declaration is inside some function
definition and when it is at top level.

julia

[Cocci] Fwd: Fwd: Re: min/max mtu field in struct net_device

[Arend van Spriel]

On 18-11-16 10:01, Arend van Spriel wrote:
> On 17-11-16 23:48, Julia Lawall wrote:
>>
>>
>> On Thu, 17 Nov 2016, Arend Van Spriel wrote:
>>
>>> Not seen a response on the backports mailing list so subscribed to the
>>> cocci mailing list and reposting it.
>>
>> Sorry to have missed seeing it.
> 
> No problem.
> 
>>>
>>> Attached is a script I want to use to backport drivers. However, the
>>> second (unnamed) rule matches for each occurence of 'struct
>>> net_device_ops'. Is there a way to disable a rule after a desired amount
>>> of matches (in my case once)?
>>
>> Here is a somewhat clunky solution:
>>

[...]

>> struct net_device_ops OPS = {
>> +      .ndo_change_mtu = __change_mtu,
>>        ...
>> };
>>
>> -------------------------
> 
> Big thanks. Have not tried it yet, but do you want me to add your
> Signed-off-by given that you mostly solved the problem.

Hi Julia,

It works so let me know if you want me to add the Signed-off-by giving
you proper credit when I submit it as patch to the backports project.

Regards,
Arend

[Cocci] Fwd: Fwd: Re: min/max mtu field in struct net_device

[Julia Lawall]

On Sat, 19 Nov 2016, Arend van Spriel wrote:

> On 18-11-16 10:01, Arend van Spriel wrote:
> > On 17-11-16 23:48, Julia Lawall wrote:
> >>
> >>
> >> On Thu, 17 Nov 2016, Arend Van Spriel wrote:
> >>
> >>> Not seen a response on the backports mailing list so subscribed to the
> >>> cocci mailing list and reposting it.
> >>
> >> Sorry to have missed seeing it.
> >
> > No problem.
> >
> >>>
> >>> Attached is a script I want to use to backport drivers. However, the
> >>> second (unnamed) rule matches for each occurence of 'struct
> >>> net_device_ops'. Is there a way to disable a rule after a desired amount
> >>> of matches (in my case once)?
> >>
> >> Here is a somewhat clunky solution:
> >>
>
> [...]
>
> >> struct net_device_ops OPS = {
> >> +      .ndo_change_mtu = __change_mtu,
> >>        ...
> >> };
> >>
> >> -------------------------
> >
> > Big thanks. Have not tried it yet, but do you want me to add your
> > Signed-off-by given that you mostly solved the problem.
>
> Hi Julia,
>
> It works so let me know if you want me to add the Signed-off-by giving
> you proper credit when I submit it as patch to the backports project.

Yes, you can add the Signed-off-by.  Thanks.

julia