hamradio: avoid null deref

If dev == NULL we shouldn't dereference it.

@@ -283,7 +283,6 @@ static netdev_tx_t bpq_xmit(struct sk_bu
 	bpq = netdev_priv(dev);
 
 	if ((dev = bpq_get_ether_dev(dev)) == NULL) {
-		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}

Dan Carpenter wrote, On 12/23/2009 02:25 PM:

From: Jarek Poplawski <[email protected]>
Date: Wed, 23 Dec 2009 18:47:46 +0100

And that is definitely the intent of the code here, to bump the statistic in the original device object.

Bump the stats on the original dev not on the newly assigned NULL version of dev.

@@ -282,11 +282,12 @@ static netdev_tx_t bpq_xmit(struct sk_bu
 
 	bpq = netdev_priv(dev);
 
-	if ((dev = bpq_get_ether_dev(dev)) == NULL) {
+	if (!bpq->ethdev) {
 		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
+	dev = bpq_get_ether_dev(dev);
 
 	skb->protocol = ax25_type_trans(skb, dev);
 	skb_reset_network_header(skb);

From: Dan Carpenter <[email protected]>
Date: Sat, 26 Dec 2009 14:38:12 +0200

This doesn't look real nice.

The bpq_get_ether_dev() abstraction exists so that the details of bpq->this and bpq->that are hidden behind it.

Exposing those details inline just to fix this bug makes the abstraction significantly less useful, and the code more ugly.

Please just create an "orig_dev" pointer to save the original device in, and use it to fix this problem properly.

That way you only fetch the bpq ether device pointer via the abstraction interface.

And BTW, this is how other reviewers told you to implement this fix. :-)

Thanks.

This should address the problems in version 1 (lazy) and version 2 (ugly).

Bump the stats on orig_dev not on the newly assigned NULL dev variable.

@@ -248,6 +248,7 @@ static netdev_tx_t bpq_xmit(struct sk_bu
 {
 	unsigned char *ptr;
 	struct bpqdev *bpq;
+	struct net_device *orig_dev;
 	int size;
 
 	/*

@@ -282,8 +283,9 @@ static netdev_tx_t bpq_xmit(struct sk_bu
 
 	bpq = netdev_priv(dev);
 
+	orig_dev = dev;
 	if ((dev = bpq_get_ether_dev(dev)) == NULL) {
-		dev->stats.tx_dropped++;
+		orig_dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}

From: Dan Carpenter <[email protected]>
Date: Mon, 28 Dec 2009 18:54:55 +0200

Applied.