Patchwork [1/3] if.h: add IFF_BRIDGE_RESTRICTED flag

login
register
mail settings
Submitter Antonio Quartulli
Date April 8, 2013, 5:41 p.m.
Message ID <1365442863-32394-2-git-send-email-antonio@open-mesh.com>
Download mbox | patch
Permalink /patch/234851/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Antonio Quartulli - April 8, 2013, 5:41 p.m.
This new flag tells whether a network device has to be
considered as restricted in the new bridge forwarding logic.

Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---
 include/uapi/linux/if.h | 1 +
 net/core/dev.c          | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
stephen hemminger - April 8, 2013, 6:58 p.m.
The standard way to do this is to use netfilter. Considering the
additional device flags and skb flag changes, I am not sure that your
method is better.

On Mon, Apr 8, 2013 at 10:41 AM, Antonio Quartulli
<antonio@open-mesh.com> wrote:
> This new flag tells whether a network device has to be
> considered as restricted in the new bridge forwarding logic.
>
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> ---
>  include/uapi/linux/if.h | 1 +
>  net/core/dev.c          | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> index 1ec407b..5c3a9bd 100644
> --- a/include/uapi/linux/if.h
> +++ b/include/uapi/linux/if.h
> @@ -83,6 +83,7 @@
>  #define IFF_SUPP_NOFCS 0x80000         /* device supports sending custom FCS */
>  #define IFF_LIVE_ADDR_CHANGE 0x100000  /* device supports hardware address
>                                          * change when it's running */
> +#define IFF_BRIDGE_RESTRICTED 0x200000 /* device is bridge-restricted */
>
>
>  #define IF_GET_IFACE   0x0001          /* for querying only */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3655ff9..49eafc8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4627,7 +4627,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags)
>
>         dev->flags = (flags & (IFF_DEBUG | IFF_NOTRAILERS | IFF_NOARP |
>                                IFF_DYNAMIC | IFF_MULTICAST | IFF_PORTSEL |
> -                              IFF_AUTOMEDIA)) |
> +                              IFF_AUTOMEDIA | IFF_BRIDGE_RESTRICTED)) |
>                      (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC |
>                                     IFF_ALLMULTI));
>
> --
> 1.8.1.5
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Quartulli - April 9, 2013, 6:33 a.m.
Hi Stephen,

thank you for your reply.

On Mon, Apr 08, 2013 at 11:58:48 -0700, Stephen Hemminger wrote:
> The standard way to do this is to use netfilter. Considering the
> additional device flags and skb flag changes, I am not sure that your
> method is better.
> 

The point is that netfilter would not help me in "distributing" this policy
remotely over a generic layer2 network.

Using these flags, instead, I can make other modules (e.g. batman-adv) notice
that the skb has been marked and then react using their own logic.

If netfilter (at the bridge level) could "mark" the skbs somehow then I could use
it for this purpose. But I don't think this is really possible.

Cheers,

> On Mon, Apr 8, 2013 at 10:41 AM, Antonio Quartulli
> <antonio@open-mesh.com> wrote:
> > This new flag tells whether a network device has to be
> > considered as restricted in the new bridge forwarding logic.
> >
> > Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> > ---
> >  include/uapi/linux/if.h | 1 +
> >  net/core/dev.c          | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> > index 1ec407b..5c3a9bd 100644
> > --- a/include/uapi/linux/if.h
> > +++ b/include/uapi/linux/if.h
> > @@ -83,6 +83,7 @@
> >  #define IFF_SUPP_NOFCS 0x80000         /* device supports sending custom FCS */
> >  #define IFF_LIVE_ADDR_CHANGE 0x100000  /* device supports hardware address
> >                                          * change when it's running */
> > +#define IFF_BRIDGE_RESTRICTED 0x200000 /* device is bridge-restricted */
> >
> >
> >  #define IF_GET_IFACE   0x0001          /* for querying only */
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3655ff9..49eafc8 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4627,7 +4627,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags)
> >
> >         dev->flags = (flags & (IFF_DEBUG | IFF_NOTRAILERS | IFF_NOARP |
> >                                IFF_DYNAMIC | IFF_MULTICAST | IFF_PORTSEL |
> > -                              IFF_AUTOMEDIA)) |
> > +                              IFF_AUTOMEDIA | IFF_BRIDGE_RESTRICTED)) |
> >                      (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC |
> >                                     IFF_ALLMULTI));
> >
> > --
> > 1.8.1.5
> >
Antonio Quartulli - April 9, 2013, 7:56 a.m.
On Mon, Apr 08, 2013 at 11:58:48 -0700, Stephen Hemminger wrote:
> The standard way to do this is to use netfilter. Considering the
> additional device flags and skb flag changes, I am not sure that your
> method is better.

To make it a bit more clear:

1) the skb flag will be used on the "receiving end-point" by batman-adv to mark
received packets and so instruct the bridge to do not forward them to restricted
interfaces.

2) the IFF_ flag is used by batman-adv on the "sending side" to determine
whether a packet has been originated by a restricted interface and so instruct
the remote endpoint to mark the skb when received.

3) to make the bridge code general enough, I decided to let it mark packets
coming from restricted interfaces as well so that it can also apply the policy
at 1) locally, without any further setting. The logic described in 1) is
therefore applied by the bridge even for local packets (not passing through
batman-adv)



Point 3) is the only one where netfilter might help. But using two mechanism to
achieve one goal looked not sane to me and therefore I decided to to do it this
way. And actually the code allowing point 3 is only:

+       skb->bridge_restricted = !!(skb->dev->flags & IFF_BRIDGE_RESTRICTED);


I hope this summary did not create further confusion :)

Thanks,

> 
> On Mon, Apr 8, 2013 at 10:41 AM, Antonio Quartulli
> <antonio@open-mesh.com> wrote:
> > This new flag tells whether a network device has to be
> > considered as restricted in the new bridge forwarding logic.
> >
> > Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> > ---
> >  include/uapi/linux/if.h | 1 +
> >  net/core/dev.c          | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> > index 1ec407b..5c3a9bd 100644
> > --- a/include/uapi/linux/if.h
> > +++ b/include/uapi/linux/if.h
> > @@ -83,6 +83,7 @@
> >  #define IFF_SUPP_NOFCS 0x80000         /* device supports sending custom FCS */
> >  #define IFF_LIVE_ADDR_CHANGE 0x100000  /* device supports hardware address
> >                                          * change when it's running */
> > +#define IFF_BRIDGE_RESTRICTED 0x200000 /* device is bridge-restricted */
> >
> >
> >  #define IF_GET_IFACE   0x0001          /* for querying only */
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3655ff9..49eafc8 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4627,7 +4627,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags)
> >
> >         dev->flags = (flags & (IFF_DEBUG | IFF_NOTRAILERS | IFF_NOARP |
> >                                IFF_DYNAMIC | IFF_MULTICAST | IFF_PORTSEL |
> > -                              IFF_AUTOMEDIA)) |
> > +                              IFF_AUTOMEDIA | IFF_BRIDGE_RESTRICTED)) |
> >                      (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC |
> >                                     IFF_ALLMULTI));
> >
> > --
> > 1.8.1.5
> >
Jamal Hadi Salim - April 9, 2013, 12:57 p.m.
Hi,

Consider using tc for this.
You can tag the packet using skb mark on the receiving end point,
match them on the bridge and execute actions not to forward them.

cheers,
jamal

On 13-04-09 03:56 AM, Antonio Quartulli wrote:
> On Mon, Apr 08, 2013 at 11:58:48 -0700, Stephen Hemminger wrote:
>> The standard way to do this is to use netfilter. Considering the
>> additional device flags and skb flag changes, I am not sure that your
>> method is better.
>
> To make it a bit more clear:
>
> 1) the skb flag will be used on the "receiving end-point" by batman-adv to mark
> received packets and so instruct the bridge to do not forward them to restricted
> interfaces.
>
> 2) the IFF_ flag is used by batman-adv on the "sending side" to determine
> whether a packet has been originated by a restricted interface and so instruct
> the remote endpoint to mark the skb when received.
>
> 3) to make the bridge code general enough, I decided to let it mark packets
> coming from restricted interfaces as well so that it can also apply the policy
> at 1) locally, without any further setting. The logic described in 1) is
> therefore applied by the bridge even for local packets (not passing through
> batman-adv)
>
>
>
> Point 3) is the only one where netfilter might help. But using two mechanism to
> achieve one goal looked not sane to me and therefore I decided to to do it this
> way. And actually the code allowing point 3 is only:
>
> +       skb->bridge_restricted = !!(skb->dev->flags & IFF_BRIDGE_RESTRICTED);
>
>
> I hope this summary did not create further confusion :)
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Quartulli - April 9, 2013, 1:51 p.m.
On Tue, Apr 09, 2013 at 05:57:45 -0700, Jamal Hadi Salim wrote:
> Hi,
> 
> Consider using tc for this.
> You can tag the packet using skb mark on the receiving end point,
> match them on the bridge and execute actions not to forward them.


Does this work at the bridge level? A packet entering a port and going out from
another one can be affected by tc/mark?


> 
> cheers,
> jamal
> 
> On 13-04-09 03:56 AM, Antonio Quartulli wrote:
> > On Mon, Apr 08, 2013 at 11:58:48 -0700, Stephen Hemminger wrote:
> >> The standard way to do this is to use netfilter. Considering the
> >> additional device flags and skb flag changes, I am not sure that your
> >> method is better.
> >
> > To make it a bit more clear:
> >
> > 1) the skb flag will be used on the "receiving end-point" by batman-adv to mark
> > received packets and so instruct the bridge to do not forward them to restricted
> > interfaces.
> >
> > 2) the IFF_ flag is used by batman-adv on the "sending side" to determine
> > whether a packet has been originated by a restricted interface and so instruct
> > the remote endpoint to mark the skb when received.
> >
> > 3) to make the bridge code general enough, I decided to let it mark packets
> > coming from restricted interfaces as well so that it can also apply the policy
> > at 1) locally, without any further setting. The logic described in 1) is
> > therefore applied by the bridge even for local packets (not passing through
> > batman-adv)
> >
> >
> >
> > Point 3) is the only one where netfilter might help. But using two mechanism to
> > achieve one goal looked not sane to me and therefore I decided to to do it this
> > way. And actually the code allowing point 3 is only:
> >
> > +       skb->bridge_restricted = !!(skb->dev->flags & IFF_BRIDGE_RESTRICTED);
> >
> >
> > I hope this summary did not create further confusion :)
> >
>
Jamal Hadi Salim - April 9, 2013, 3:49 p.m.
On 13-04-09 09:51 AM, Antonio Quartulli wrote:

>
> Does this work at the bridge level? A packet entering a port and going out from
> another one can be affected by tc/mark?

Yes of course. And on any construct that looks like a netdev (tunnels etc).

cheers,
jamal

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Quartulli - April 10, 2013, 4:54 p.m.
Hi Jamal, all,

On Tue, Apr 09, 2013 at 08:49:17 -0700, Jamal Hadi Salim wrote:
> On 13-04-09 09:51 AM, Antonio Quartulli wrote:
> 
> >
> > Does this work at the bridge level? A packet entering a port and going out from
> > another one can be affected by tc/mark?
> 
> Yes of course. And on any construct that looks like a netdev (tunnels etc).
> 

Thanks for your hints. After having struggled a bit I found out how to do it
using ebtables and the mark target :)

Thanks a Lot!

These patches seem to be useless now

Cheers,
stephen hemminger - April 10, 2013, 8:46 p.m.
On Wed, 10 Apr 2013 18:54:34 +0200
Antonio Quartulli <antonio@open-mesh.com> wrote:

> Hi Jamal, all,
> 
> On Tue, Apr 09, 2013 at 08:49:17 -0700, Jamal Hadi Salim wrote:
> > On 13-04-09 09:51 AM, Antonio Quartulli wrote:
> > 
> > >
> > > Does this work at the bridge level? A packet entering a port and going out from
> > > another one can be affected by tc/mark?
> > 
> > Yes of course. And on any construct that looks like a netdev (tunnels etc).
> > 
> 
> Thanks for your hints. After having struggled a bit I found out how to do it
> using ebtables and the mark target :)
> 
> Thanks a Lot!
> 
> These patches seem to be useless now
> 
> Cheers,
> 

Come back again, though. The ebtables method offers more flexibility which can
be a good or bad thing...
Antonio Quartulli - April 11, 2013, 10:56 a.m.
On Wed, Apr 10, 2013 at 01:46:09PM -0700, Stephen Hemminger wrote:
> On Wed, 10 Apr 2013 18:54:34 +0200
> Antonio Quartulli <antonio@open-mesh.com> wrote:
> 
> > Hi Jamal, all,
> > 
> > On Tue, Apr 09, 2013 at 08:49:17 -0700, Jamal Hadi Salim wrote:
> > > On 13-04-09 09:51 AM, Antonio Quartulli wrote:
> > > 
> > > >
> > > > Does this work at the bridge level? A packet entering a port and going out from
> > > > another one can be affected by tc/mark?
> > > 
> > > Yes of course. And on any construct that looks like a netdev (tunnels etc).
> > > 
> > 
> > Thanks for your hints. After having struggled a bit I found out how to do it
> > using ebtables and the mark target :)
> > 
> > Thanks a Lot!
> > 
> Come back again, though. The ebtables method offers more flexibility which can
> be a good or bad thing...

I just realised that :)

By installing ebtables (meaning modules + userspace tool) my iperf test result
drops from 81Mbps to 66Mbps: former without, latter with ebtables module enabled.
I did this test between two devices connected with Fast Ethernet.

I thought that most of the code is in netfilter, so shared with iptables, hence
I expected a reasonable overhead why this is much worse.

Does anybody have a clue about this? I should probably start a new thread on the
netfilter mailing list.

However this problem makes ebtables unusable at all.

Suggestions are welcome :)

Cheers,
Jamal Hadi Salim - April 11, 2013, 11:03 a.m.
On 13-04-11 06:56 AM, Antonio Quartulli wrote:

> I just realised that :)
>
> By installing ebtables (meaning modules + userspace tool) my iperf test result
> drops from 81Mbps to 66Mbps: former without, latter with ebtables module enabled.
> I did this test between two devices connected with Fast Ethernet.
>


Please try tc like i said earlier ;->

cheers,
jamal

> I thought that most of the code is in netfilter, so shared with iptables, hence
> I expected a reasonable overhead why this is much worse.
>
> Does anybody have a clue about this? I should probably start a new thread on the
> netfilter mailing list.
>
> However this problem makes ebtables unusable at all.
>
> Suggestions are welcome :)
>
> Cheers,
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index 1ec407b..5c3a9bd 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -83,6 +83,7 @@ 
 #define IFF_SUPP_NOFCS	0x80000		/* device supports sending custom FCS */
 #define IFF_LIVE_ADDR_CHANGE 0x100000	/* device supports hardware address
 					 * change when it's running */
+#define IFF_BRIDGE_RESTRICTED 0x200000	/* device is bridge-restricted */
 
 
 #define IF_GET_IFACE	0x0001		/* for querying only */
diff --git a/net/core/dev.c b/net/core/dev.c
index 3655ff9..49eafc8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4627,7 +4627,7 @@  int __dev_change_flags(struct net_device *dev, unsigned int flags)
 
 	dev->flags = (flags & (IFF_DEBUG | IFF_NOTRAILERS | IFF_NOARP |
 			       IFF_DYNAMIC | IFF_MULTICAST | IFF_PORTSEL |
-			       IFF_AUTOMEDIA)) |
+			       IFF_AUTOMEDIA | IFF_BRIDGE_RESTRICTED)) |
 		     (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC |
 				    IFF_ALLMULTI));