| Message ID | 20131212134159.5bc985a9@nehalam.linuxnetplumber.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 12 Dec 2013 13:41:59 -0800 > When an Ethernet device is enslaved to a bridge, and the bridge STP > detects loss of carrier (or operational state down), then normally > packet reception is blocked. > > This breaks control applications like WPA which maybe expecting to > receive packets to negotiate to bring link up. The bridge needs to > block forwarding packets from these disabled ports, but there is no > hard requirement to not allow local packet delivery. > > In this special case, packets are not forwarded (local delivery only), > and only packet directed at the address of the Ethernet device are > accepted (no promiscuous or other ports in bridge). > > The existing code already allowed link-local-address packets in > which is what STP uses to communicate with other bridges. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > Signed-off-by: Felix Fietkau <nbd@openwrt.org> I think this change needs to be more careful about the setting of *pskb. It should not be assigned if we return RX_HANDLER_CONSUMED. -- 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
On Thu, 12 Dec 2013 17:26:37 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Thu, 12 Dec 2013 13:41:59 -0800 > > > When an Ethernet device is enslaved to a bridge, and the bridge STP > > detects loss of carrier (or operational state down), then normally > > packet reception is blocked. > > > > This breaks control applications like WPA which maybe expecting to > > receive packets to negotiate to bring link up. The bridge needs to > > block forwarding packets from these disabled ports, but there is no > > hard requirement to not allow local packet delivery. > > > > In this special case, packets are not forwarded (local delivery only), > > and only packet directed at the address of the Ethernet device are > > accepted (no promiscuous or other ports in bridge). > > > > The existing code already allowed link-local-address packets in > > which is what STP uses to communicate with other bridges. > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > Signed-off-by: Felix Fietkau <nbd@openwrt.org> > > I think this change needs to be more careful about the setting of > *pskb. It should not be assigned if we return RX_HANDLER_CONSUMED. It was already in the existing code path for link local. -- 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
From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 12 Dec 2013 15:05:23 -0800 > On Thu, 12 Dec 2013 17:26:37 -0500 (EST) > David Miller <davem@davemloft.net> wrote: > >> From: Stephen Hemminger <stephen@networkplumber.org> >> Date: Thu, 12 Dec 2013 13:41:59 -0800 >> >> > When an Ethernet device is enslaved to a bridge, and the bridge STP >> > detects loss of carrier (or operational state down), then normally >> > packet reception is blocked. >> > >> > This breaks control applications like WPA which maybe expecting to >> > receive packets to negotiate to bring link up. The bridge needs to >> > block forwarding packets from these disabled ports, but there is no >> > hard requirement to not allow local packet delivery. >> > >> > In this special case, packets are not forwarded (local delivery only), >> > and only packet directed at the address of the Ethernet device are >> > accepted (no promiscuous or other ports in bridge). >> > >> > The existing code already allowed link-local-address packets in >> > which is what STP uses to communicate with other bridges. >> > >> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >> > Signed-off-by: Felix Fietkau <nbd@openwrt.org> >> >> I think this change needs to be more careful about the setting of >> *pskb. It should not be assigned if we return RX_HANDLER_CONSUMED. > > It was already in the existing code path for link local. That's not true, the "*pskb = skb;" assignment was only done when the code retuned RX_HANDLER_PASS. You are changing it to unconditionally make this assignment. -- 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
--- a/net/bridge/br_input.c 2013-12-06 16:49:53.663016412 -0800 +++ b/net/bridge/br_input.c 2013-12-08 12:27:04.736572830 -0800 @@ -152,6 +152,16 @@ static int br_handle_local_finish(struct return 0; /* process further */ } +/* Deliver packet to local host only */ +static rx_handler_result_t br_local_only(struct sk_buff *skb) +{ + if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev, + NULL, br_handle_local_finish)) + return RX_HANDLER_CONSUMED; /* consumed by filter */ + else + return RX_HANDLER_PASS; /* continue processing */ +} + /* * Return NULL if skb is handled * note: already called with rcu_read_lock @@ -206,14 +216,8 @@ rx_handler_result_t br_handle_frame(stru goto forward; } - /* Deliver packet to local host only */ - if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev, - NULL, br_handle_local_finish)) { - return RX_HANDLER_CONSUMED; /* consumed by filter */ - } else { - *pskb = skb; - return RX_HANDLER_PASS; /* continue processing */ - } + *pskb = skb; + return br_local_only(skb); } forward: @@ -235,6 +239,13 @@ forward: NF_HOOK(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL, br_handle_frame_finish); break; + + case BR_STATE_DISABLED: + if (skb->pkt_type == PACKET_HOST) { + *pskb = skb; + return br_local_only(skb); + } + /* fall through */ default: drop: kfree_skb(skb);