[PATCH] dp83640: don't recieve time stamps twice

[PATCH] dp83640: don't recieve time stamps twice

[Dan Carpenter]

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.

Fixes: 4b063258ab93 ("dp83640: Delay scheduled work.")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index e2460a57e4b1..ed0d10f54f26 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -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;

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Tue, Apr 18, 2017 at 10:14:26PM +0300, Dan Carpenter wrote:
> 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.

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.
 
> 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.

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

Re: [PATCH] dp83640: don't recieve time stamps twice

[Sørensen, Stefan]

On Tue, 2017-04-18 at 22:14 +0300, Dan Carpenter wrote:
> 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.

Sorry for not getting back earlier, I have swamped with other stuff.

You are correct that these lines was added accidentally.

Acked-by: Stefan Sørensen <stefan.sorensen@spectralink.com>

> 
> Fixes: 4b063258ab93 ("dp83640: Delay scheduled work.")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index e2460a57e4b1..ed0d10f54f26 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -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;

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
> You are correct that these lines was added accidentally.

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

---
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index e2460a5..593e533 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -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;

Re: [PATCH] dp83640: don't recieve time stamps twice

[Dan Carpenter]

On Wed, Apr 19, 2017 at 12:31:35PM +0200, Richard Cochran wrote:
> On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
> > You are correct that these lines was added accidentally.
> 
> 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...
> 

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

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH] dp83640: don't recieve time stamps twice

[Sørensen, Stefan]

On Wed, 2017-04-19 at 12:31 +0200, Richard Cochran wrote:
> 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...


I agree that this is a better way to fix it - I think that this was the
purpose of the patch that got halfway mixed in.

@@ -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);

Here we still hold rx_lock while during the callback, wouldn't it be
beneficial to release that first?

Stefan

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Wed, Apr 19, 2017 at 11:28:34AM +0000, Sørensen, Stefan wrote:
> Here we still hold rx_lock while during the callback, wouldn't it be
> beneficial to release that first?

Yes, your are right.

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

Thanks,
Richard

Re: [PATCH] dp83640: don't recieve time stamps twice

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Wed, 19 Apr 2017 12:31:35 +0200

> On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
>> You are correct that these lines was added accidentally.
> 
> 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...

Oops, I read this too late.

I already applied and pushed out Dan's fix.

Re: [PATCH] dp83640: don't recieve time stamps twice

[Richard Cochran]

On Thu, Apr 20, 2017 at 04:02:27PM -0400, David Miller wrote:
> Oops, I read this too late.
> 
> I already applied and pushed out Dan's fix.

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

Thanks,
Richard