lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 17 Jan 2015 17:05:08 -0800
From:	Scott Feldman <sfeldma@...il.com>
To:	Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	"David S. Miller" <davem@...emloft.net>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Jiří Pírko <jiri@...nulli.us>,
	"Arad, Ronen" <ronen.arad@...el.com>, Thomas Graf <tgraf@...g.ch>,
	john fastabend <john.fastabend@...il.com>,
	"vyasevic@...hat.com" <vyasevic@...hat.com>,
	Netdev <netdev@...r.kernel.org>,
	Wilson Kok <wkok@...ulusnetworks.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>
Subject: Re: [RFC PATCH net-next] bridge: ability to disable forwarding on a port

On Fri, Jan 16, 2015 at 11:32 PM,  <roopa@...ulusnetworks.com> wrote:
> From: Roopa Prabhu <roopa@...ulusnetworks.com>
>
> On a Linux bridge with bridge forwarding offloaded to a switch ASIC,
> there is a need to not re-forward the frames that come up to the
> kernel in software.
>
> Typically these are broadcast or multicast packets forwarded by the
> hardware to multiple destination ports including sending a copy of
> the packet to the kernel (e.g. an arp broadcast).
> The bridge driver will try to forward the packet again, resulting in
> two copies of the same packet.
>
> These packets can also come up to the kernel for logging when they hit
> a LOG acl in hardware.
>
> This patch makes forwarding a flag on the port similar to
> learn and flood and drops the packet just before forwarding.
> (The forwarding disable on a bridge is tested to work on our boxes.
> The bridge port flag addition is only compile tested.
> This will need to be further refined to cover cases where a non-switch port
> is bridged to a switch port etc. We will submit more patches to cover
> all cases if we agree on this approach).

Good topic to bring up, thanks for proposing a patch.  There is indeed
duplicate pkts sent out in the case where both the bridge and the
offloaded device are flooding these non-unicast pkts, such as ARP
requests.  We do have per-port control today over unicast flooding
using BR_FLOOD (IFLA_BRPORT_UNICAST_FLOOD).

As you point out, this doesn't solve the case for non-offloaded ports
bridged with switch ports.  If this port setting is enabled on an
offloaded switch port, for example, the non-offloaded port can't get
an ARP request resolved, if the MAC is behind the offloaded switch
port.  But do we care?  Is there a use-case for this one, mixing
offloaded and non-offloaded ports in a bridge?

>
> Other ways to solve the same problem could be to:
> - use the offload feature flag on these switch ports to avoid the
> re-forward:
> https://www.marc.info/?l=linux-netdev&m=141820235010603&w=2
>
> - Or the switch driver can mark or set a flag in the skb, which the bridge
> driver can use to avoid a re-forward.
>
> Signed-off-by: Wilson Kok <wkok@...ulusnetworks.com>
> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
> ---
>  include/linux/if_bridge.h    |    3 ++-
>  include/uapi/linux/if_link.h |    1 +
>  net/bridge/br_forward.c      |   13 +++++++++++++
>  net/bridge/br_if.c           |    2 +-
>  net/bridge/br_netlink.c      |    4 +++-
>  net/bridge/br_sysfs_if.c     |    1 +
>  net/core/rtnetlink.c         |    4 +++-
>  7 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 0a8ce76..c79f4eb 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -40,10 +40,11 @@ struct br_ip_list {
>  #define BR_ADMIN_COST          BIT(4)
>  #define BR_LEARNING            BIT(5)
>  #define BR_FLOOD               BIT(6)
> -#define BR_AUTO_MASK           (BR_FLOOD | BR_LEARNING)
>  #define BR_PROMISC             BIT(7)
>  #define BR_PROXYARP            BIT(8)
>  #define BR_LEARNING_SYNC       BIT(9)
> +#define BR_FORWARD             BIT(10)

The name BR_FORWARD might confuse people thinking this is related to
STP FORWARDING state.  We have BR_FLOOD for unknown unicast flooding.
How about renaming BR_FLOOD to BR_FLOOD_UNICAST and add
BR_FLOOD_BROADCAST?  So you would have:

  IFLA_BRPORT_UNICAST_FLOOD           BR_FLOOD_UNICAST        /* flood
unknown unicast traffic to port */
  IFLA_BRPORT_BROADCAST_FLOOD    BR_FLOOD_BROADCAST  /* flood
bcast/mcast traffic to port */

> +#define BR_AUTO_MASK           (BR_FLOOD | BR_LEARNING | BR_FORWARD)
>
>  extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index f7d0d2d..d394625 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -246,6 +246,7 @@ enum {
>         IFLA_BRPORT_UNICAST_FLOOD, /* flood unicast traffic */
>         IFLA_BRPORT_PROXYARP,   /* proxy ARP */
>         IFLA_BRPORT_LEARNING_SYNC, /* mac learning sync from device */
> +       IFLA_BRPORT_FORWARD,    /* enable forwarding on a device */
>         __IFLA_BRPORT_MAX
>  };
>  #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index f96933a..98c41c8 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -81,10 +81,23 @@ static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
>                 br_forward_finish);
>  }
>
> +int br_hw_forward_finish(struct sk_buff *skb)
> +{
> +       kfree_skb(skb);
> +
> +       return 0;
> +}
> +
>  static void __br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
>  {
>         struct net_device *indev;
>
> +       if (!(to->flags & BR_FORWARD)) {
> +               NF_HOOK(NFPROTO_BRIDGE, NF_BR_FORWARD, skb, skb->dev, to->dev,
> +                       br_hw_forward_finish);
> +               return;
> +       }
> +

Seems you should make the (flags & BR_FORWARD) check earlier, before
skb cloning, in br_flood(), alongside the (flags & BR_FLOOD) check.

Also, the above code is skipping some vlan checks (br_handle_vlan).

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

Powered by blists - more mailing lists