bonding: don't increase rx_dropped after processing LACPDUs

On Wed, May 02, 2012 at 10:36:49PM +0200, Eric Dumazet wrote:
> > +			if (ret == RX_HANDLER_CONSUMED)
> > +				kfree_skb(skb);
> 
> After this point, you have use after free :
> 
> if (bond_should_deliver_exact_match(skb, slave, bond)) { 
> 	...
> }
> skb->dev = bond->dev;

Thanks for spotting this! Let's just return immediately at that
point. Fixed version below:

Signed-off-by: Jiri Bohac <jbohac@suse.cz>

Jiri Bohac <jbohac@suse.cz> wrote:

>On Wed, May 02, 2012 at 10:36:49PM +0200, Eric Dumazet wrote:
>> > +			if (ret == RX_HANDLER_CONSUMED)
>> > +				kfree_skb(skb);
>> 
>> After this point, you have use after free :
>> 
>> if (bond_should_deliver_exact_match(skb, slave, bond)) { 
>> 	...
>> }
>> skb->dev = bond->dev;
>
>Thanks for spotting this! Let's just return immediately at that
>point. Fixed version below:

	Won't this make it impossible to bind a PF_PACKET socket to
sll_protocol == ETH_P_SLOW and see the LACPDUs, but only when bonding is
running 802.3ad?  This because the ptype_all check in
__netif_receive_skb happens before the rx_handler, but the ptype_base
check (bound packet socket, for example) happens after.  Currently,
libpcap looks to bind to ETH_P_ALL, so it won't be affected.

	If so, is that something we care about?

	-J


>Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -2173,9 +2173,10 @@ re_arm:
>  * received frames (loopback). Since only the payload is given to this
>  * function, it check for loopback.
>  */
>-static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
>+static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
> {
> 	struct port *port;
>+	int ret = RX_HANDLER_ANOTHER;
>
> 	if (length >= sizeof(struct lacpdu)) {
>
>@@ -2184,11 +2185,12 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> 		if (!port->slave) {
> 			pr_warning("%s: Warning: port of slave %s is uninitialized\n",
> 				   slave->dev->name, slave->dev->master->name);
>-			return;
>+			return ret;
> 		}
>
> 		switch (lacpdu->subtype) {
> 		case AD_TYPE_LACPDU:
>+			ret = RX_HANDLER_CONSUMED;
> 			pr_debug("Received LACPDU on port %d\n",
> 				 port->actor_port_number);
> 			/* Protect against concurrent state machines */
>@@ -2198,6 +2200,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> 			break;
>
> 		case AD_TYPE_MARKER:
>+			ret = RX_HANDLER_CONSUMED;
> 			// No need to convert fields to Little Endian since we don't use the marker's fields.
>
> 			switch (((struct bond_marker *)lacpdu)->tlv_type) {
>@@ -2219,6 +2222,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> 			}
> 		}
> 	}
>+	return ret;
> }
>
> /**
>@@ -2456,18 +2460,20 @@ out:
> 	return NETDEV_TX_OK;
> }
>
>-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> 			  struct slave *slave)
> {
>+	int ret = RX_HANDLER_ANOTHER;
> 	if (skb->protocol != PKT_TYPE_LACPDU)
>-		return;
>+		return ret;
>
> 	if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
>-		return;
>+		return ret;
>
> 	read_lock(&bond->lock);
>-	bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
>+	ret = bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
> 	read_unlock(&bond->lock);
>+	return ret;
> }
>
> /*
>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>index 235b2cc..5ee7e3c 100644
>--- a/drivers/net/bonding/bond_3ad.h
>+++ b/drivers/net/bonding/bond_3ad.h
>@@ -274,7 +274,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
> void bond_3ad_handle_link_change(struct slave *slave, char link);
> int  bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
>-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> 			  struct slave *slave);
> int bond_3ad_set_carrier(struct bonding *bond);
> void bond_3ad_update_lacp_rate(struct bonding *bond);
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 62d2409..0a0f4a6 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1444,8 +1444,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> 	struct sk_buff *skb = *pskb;
> 	struct slave *slave;
> 	struct bonding *bond;
>-	void (*recv_probe)(struct sk_buff *, struct bonding *,
>+	int (*recv_probe)(struct sk_buff *, struct bonding *,
> 				struct slave *);
>+	int ret = RX_HANDLER_ANOTHER;
>
> 	skb = skb_share_check(skb, GFP_ATOMIC);
> 	if (unlikely(!skb))
>@@ -1464,8 +1465,12 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> 		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>
> 		if (likely(nskb)) {
>-			recv_probe(nskb, bond, slave);
>+			ret = recv_probe(nskb, bond, slave);
> 			dev_kfree_skb(nskb);
>+			if (ret == RX_HANDLER_CONSUMED) {
>+				kfree_skb(skb);
>+				return ret;
>+			}
> 		}
> 	}
>
>@@ -1487,7 +1492,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> 		memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
> 	}
>
>-	return RX_HANDLER_ANOTHER;
>+	return ret;
> }
>
> /* enslave device <slave> to bond device <master> */
>@@ -2723,7 +2728,7 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
> 	}
> }
>
>-static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>+static int bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> 			 struct slave *slave)
> {
> 	struct arphdr *arp;
>@@ -2731,7 +2736,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> 	__be32 sip, tip;
>
> 	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
>-		return;
>+		return RX_HANDLER_ANOTHER;
>
> 	read_lock(&bond->lock);
>
>@@ -2776,6 +2781,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>
> out_unlock:
> 	read_unlock(&bond->lock);
>+	return RX_HANDLER_ANOTHER;
> }
>
> /*
>
>
>-- 
>Jiri Bohac <jbohac@suse.cz>
>SUSE Labs, SUSE CZ

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

--
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: Jiri Bohac <jbohac@suse.cz>
Date: Wed, 2 May 2012 22:51:18 +0200

> On Wed, May 02, 2012 at 10:36:49PM +0200, Eric Dumazet wrote:
>> > +			if (ret == RX_HANDLER_CONSUMED)
>> > +				kfree_skb(skb);
>> 
>> After this point, you have use after free :
>> 
>> if (bond_should_deliver_exact_match(skb, slave, bond)) { 
>> 	...
>> }
>> skb->dev = bond->dev;
> 
> Thanks for spotting this! Let's just return immediately at that
> point. Fixed version below:
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>

Please don't do this.

When you post a fixed version of a patch, post it with the full
proper commit message and signoff.

I'm not going to go back to your original posting and put that
commit message from there into the fixed patch.  That's your
job as a patch submitter, not mine.
--
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 Wed, May 02, 2012 at 04:10:39PM -0700, Jay Vosburgh wrote:
> 	Won't this make it impossible to bind a PF_PACKET socket to
> sll_protocol == ETH_P_SLOW and see the LACPDUs, but only when bonding is
> running 802.3ad? 

yes it will...

> This because the ptype_all check in
> __netif_receive_skb happens before the rx_handler, but the ptype_base
> check (bound packet socket, for example) happens after.  Currently,
> libpcap looks to bind to ETH_P_ALL, so it won't be affected.

Does it make any sense for the packet socket not to bind to
ETH_P_ALL? With all the rx_handlers interfering with the packet
(e.g. modifying skb->dev, modifying the MAC address), binding to
ETH_P_SLOW will never reliably get you the the original frame
with reliable information about the incoming device. 

> If so, is that something we care about?

I think not.

On Wed, May 02, 2012 at 07:41:05PM -0400, David Miller wrote:
> I'm not going to go back to your original posting and put that
> commit message from there into the fixed patch.  That's your
> job as a patch submitter, not mine.

Sorry! I'll resend...