dp83640: don't recieve time stamps twice

This patch is prompted by a static checker warning about a potential use after free. The concern is that netif_rx_ni() can free "skb" and we call it twice.

When I look at the commit that added this, it looks like some stray lines were added accidentally. It doesn't make sense to me that we would recieve the same data two times. I asked the author but never recieved a response.

I can't test this code, but I'm pretty sure my patch is correct.

index e2460a57e4b1..ed0d10f54f26 100644
@@ -1438,8 +1438,6 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 		skb_info->tmo = jiffies + SKB_TIMESTAMP_TIMEOUT;
 		skb_queue_tail(&dp83640->rx_queue, skb);
 		schedule_delayed_work(&dp83640->ts_work, SKB_TIMESTAMP_TIMEOUT);
-	} else {
-		netif_rx_ni(skb);
 	}
 
 	return true;

Right, the code already calls netif_rx_ni() in the list_for_each_safe() loop just above, in the case that the shhwtstamps pointer has been set.

Hm, maybe the intent was to move the call to netif_rx_ni() outside of the spin_lock_irqsave() region (which how I had it before Stefan's changes).

But calling netif_rx_ni() twice is clearly wrong.

Thanks,
Richard

T24gVHVlLCAyMDE3LTA0LTE4IGF0IDIyOjE0ICswMzAwLCBEYW4gQ2FycGVudGVyIHdyb3RlOg0K PiBUaGlzIHBhdGNoIGlzIHByb21wdGVkIGJ5IGEgc3RhdGljIGNoZWNrZXIgd2FybmluZyBhYm91 dCBhIHBvdGVudGlhbA0KPiB1c2UgYWZ0ZXIgZnJlZS7CoMKgVGhlIGNvbmNlcm4gaXMgdGhhdCBu ZXRpZl9yeF9uaSgpIGNhbiBmcmVlICJza2IiIGFuZA0KPiB3ZQ0KPiBjYWxsIGl0IHR3aWNlLg0K PiANCj4gV2hlbiBJIGxvb2sgYXQgdGhlIGNvbW1pdCB0aGF0IGFkZGVkIHRoaXMsIGl0IGxvb2tz IGxpa2Ugc29tZSBzdHJheQ0KPiBsaW5lcyB3ZXJlIGFkZGVkIGFjY2lkZW50YWxseS7CoMKgSXQg ZG9lc24ndCBtYWtlIHNlbnNlIHRvIG1lIHRoYXQgd2UNCj4gd291bGQgcmVjaWV2ZSB0aGUgc2Ft ZSBkYXRhIHR3byB0aW1lcy7CoMKgSSBhc2tlZCB0aGUgYXV0aG9yIGJ1dCBuZXZlcg0KPiByZWNp ZXZlZCBhIHJlc3BvbnNlLg0KPiANCj4gSSBjYW4ndCB0ZXN0IHRoaXMgY29kZSwgYnV0IEknbSBw cmV0dHkgc3VyZSBteSBwYXRjaCBpcyBjb3JyZWN0Lg0KDQpTb3JyeSBmb3Igbm90IGdldHRpbmcg YmFjayBlYXJsaWVyLCBJIGhhdmUgc3dhbXBlZCB3aXRoIG90aGVyIHN0dWZmLg0KDQpZb3UgYXJl IGNvcnJlY3QgdGhhdCB0aGVzZSBsaW5lcyB3YXMgYWRkZWQgYWNjaWRlbnRhbGx5Lg0KDQpBY2tl ZC1ieTogU3RlZmFuIFPDuHJlbnNlbiA8c3RlZmFuLnNvcmVuc2VuQHNwZWN0cmFsaW5rLmNvbT4N Cg0KPiANCj4gRml4ZXM6IDRiMDYzMjU4YWI5MyAoImRwODM2NDA6IERlbGF5IHNjaGVkdWxlZCB3 b3JrLiIpDQo+IFNpZ25lZC1vZmYtYnk6IERhbiBDYXJwZW50ZXIgPGRhbi5jYXJwZW50ZXJAb3Jh Y2xlLmNvbT4NCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL25ldC9waHkvZHA4MzY0MC5jIGIv ZHJpdmVycy9uZXQvcGh5L2RwODM2NDAuYw0KPiBpbmRleCBlMjQ2MGE1N2U0YjEuLmVkMGQxMGY1 NGYyNiAxMDA2NDQNCj4gLS0tIGEvZHJpdmVycy9uZXQvcGh5L2RwODM2NDAuYw0KPiArKysgYi9k cml2ZXJzL25ldC9waHkvZHA4MzY0MC5jDQo+IEBAIC0xNDM4LDggKzE0MzgsNiBAQCBzdGF0aWMg Ym9vbCBkcDgzNjQwX3J4dHN0YW1wKHN0cnVjdCBwaHlfZGV2aWNlDQo+ICpwaHlkZXYsDQo+IMKg CQlza2JfaW5mby0+dG1vID0gamlmZmllcyArIFNLQl9USU1FU1RBTVBfVElNRU9VVDsNCj4gwqAJ CXNrYl9xdWV1ZV90YWlsKCZkcDgzNjQwLT5yeF9xdWV1ZSwgc2tiKTsNCj4gwqAJCXNjaGVkdWxl X2RlbGF5ZWRfd29yaygmZHA4MzY0MC0+dHNfd29yaywNCj4gU0tCX1RJTUVTVEFNUF9USU1FT1VU KTsNCj4gLQl9IGVsc2Ugew0KPiAtCQluZXRpZl9yeF9uaShza2IpOw0KPiDCoAl9DQo+IMKgDQo+ IMKgCXJldHVybiB0cnVlOw=

Can we please fix this in another way? There is no need to hold the spin lock during the callback into the networking stack. Instead, how about the following diff, which also fixes the other call site...

Thanks,
Richard

---

index e2460a5..593e533 100644
@@ -874,14 +874,15 @@ static void decode_rxts(struct dp83640_private *dp83640,
 			shhwtstamps = skb_hwtstamps(skb);
 			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 			shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-			netif_rx_ni(skb);
 			list_add(&rxts->list, &dp83640->rxpool);
 			break;
 		}
 	}
 	spin_unlock(&dp83640->rx_queue.lock);
 
-	if (!shhwtstamps)
+	if (shhwtstamps)
+		netif_rx_ni(skb);
+	else
 		list_add_tail(&rxts->list, &dp83640->rxts);
 out:
 	spin_unlock_irqrestore(&dp83640->rx_lock, flags);

@@ -1425,7 +1426,6 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 			shhwtstamps = skb_hwtstamps(skb);
 			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 			shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-			netif_rx_ni(skb);
 			list_del_init(&rxts->list);
 			list_add(&rxts->list, &dp83640->rxpool);
 			break;

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

I'm fine with this. I hated my changelog, as well because it sounds like guess work. Could you send it with a:

regards,
dan carpenter

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

T24gV2VkLCAyMDE3LTA0LTE5IGF0IDEyOjMxICswMjAwLCBSaWNoYXJkIENvY2hyYW4gd3JvdGU6 DQo+IENhbiB3ZSBwbGVhc2UgZml4IHRoaXMgaW4gYW5vdGhlciB3YXk/wqDCoFRoZXJlIGlzIG5v IG5lZWQgdG8gaG9sZCB0aGUNCj4gc3BpbiBsb2NrIGR1cmluZyB0aGUgY2FsbGJhY2sgaW50byB0 aGUgbmV0d29ya2luZyBzdGFjay7CoMKgSW5zdGVhZCwNCj4gaG93IGFib3V0IHRoZSBmb2xsb3dp bmcgZGlmZiwgd2hpY2ggYWxzbyBmaXhlcyB0aGUgb3RoZXIgY2FsbCBzaXRlLi4uDQoNCg0KSSBh Z3JlZSB0aGF0IHRoaXMgaXMgYSBiZXR0ZXIgd2F5IHRvIGZpeCBpdCAtIEkgdGhpbmsgdGhhdCB0 aGlzIHdhcyB0aGUNCnB1cnBvc2Ugb2YgdGhlIHBhdGNoIHRoYXQgZ290IGhhbGZ3YXkgbWl4ZWQg aW4uDQoNCkBAIC04NzQsMTQgKzg3NCwxNSBAQCBzdGF0aWMgdm9pZCBkZWNvZGVfcnh0cyhzdHJ1 Y3QgZHA4MzY0MF9wcml2YXRlDQo+ICpkcDgzNjQwLA0KPiDCoAkJCXNoaHd0c3RhbXBzID0gc2ti X2h3dHN0YW1wcyhza2IpOw0KPiDCoAkJCW1lbXNldChzaGh3dHN0YW1wcywgMCwNCj4gc2l6ZW9m KCpzaGh3dHN0YW1wcykpOw0KPiDCoAkJCXNoaHd0c3RhbXBzLT5od3RzdGFtcCA9IG5zX3RvX2t0 aW1lKHJ4dHMtDQo+ID5ucyk7DQo+IC0JCQluZXRpZl9yeF9uaShza2IpOw0KPiDCoAkJCWxpc3Rf YWRkKCZyeHRzLT5saXN0LCAmZHA4MzY0MC0+cnhwb29sKTsNCj4gwqAJCQlicmVhazsNCj4gwqAJ CX0NCj4gwqAJfQ0KPiDCoAlzcGluX3VubG9jaygmZHA4MzY0MC0+cnhfcXVldWUubG9jayk7DQo+ IMKgDQo+IC0JaWYgKCFzaGh3dHN0YW1wcykNCj4gKwlpZiAoc2hod3RzdGFtcHMpDQo+ICsJCW5l dGlmX3J4X25pKHNrYik7DQo+ICsJZWxzZQ0KPiDCoAkJbGlzdF9hZGRfdGFpbCgmcnh0cy0+bGlz dCwgJmRwODM2NDAtPnJ4dHMpOw0KPiDCoG91dDoNCj4gwqAJc3Bpbl91bmxvY2tfaXJxcmVzdG9y ZSgmZHA4MzY0MC0+cnhfbG9jaywgZmxhZ3MpOw0KDQpIZXJlIHdlIHN0aWxsIGhvbGQgcnhfbG9j ayB3aGlsZSBkdXJpbmcgdGhlIGNhbGxiYWNrLCB3b3VsZG4ndCBpdCBiZQ0KYmVuZWZpY2lhbCB0 byByZWxlYXNlIHRoYXQgZmlyc3Q/DQoNClN0ZWZhbg0K

Yes, your are right.

Do you want to post a fix? If not, I will, but later on this week.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Richard Cochran <[email protected]>
Date: Wed, 19 Apr 2017 12:31:35 +0200

Oops, I read this too late.

I already applied and pushed out Dan's fix.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

No problem, i'll submit a patch on top of that to move the netif calls out of the spin-locked regions.

Thanks,
Richard