netdev - Re: [PATCH net-next 1/4] net: systemport: fix transmit locking scheme

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 6 Jun 2014 10:00:43 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	David Laight <David.Laight@...lab.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH net-next 1/4] net: systemport: fix transmit locking scheme

2014-06-06 2:29 GMT-07:00 David Laight <David.Laight@...lab.com>:
> From: Florian Fainelli
>> Our transmit locking scheme did not account for the TX ring full
>> interrupt. If a TX ring full interrupt fires while we are attempting to
>> transmit, we will cause a deadlock to occur. Fix this by making sure
>> that we properly disable interrupts while acquiring the spinlock.
>
> Presumably you mean a 'TX complete' interrupt?

There is an additional interrupt that gets fired when one of the TX
ring gets full, but it is in bcm_sysport_rx_isr() since it hooked to
the first interrupt line we have.

>
> It ought to be possible to arrange the logic so that you don't
> need to disable interrupts for the entirety of the tx reclaim
> and tx setup code.
>
> If you have an absolutely accurate count of the number of
> tx descriptors required, even the tx setup code probably only needs
> the ring's lock while it advances the write pointer.
> It can fill in the ring entries after releasing the lock.

Right, I am currently experimenting with that and plan to remove the
spinlock entirely, or grab it for as little time as possible.

>
>         David
>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 25982b02f0ea..4dfc93fe9744 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -637,10 +637,11 @@ static unsigned int bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>>                                          struct bcm_sysport_tx_ring *ring)
>>  {
>>       unsigned int released;
>> +     unsigned long flags;
>>
>> -     spin_lock(&ring->lock);
>> +     spin_lock_irqsave(&ring->lock, flags);
>>       released = __bcm_sysport_tx_reclaim(priv, ring);
>> -     spin_unlock(&ring->lock);
>> +     spin_unlock_irqrestore(&ring->lock, flags);
>>
>>       return released;
>>  }
>> @@ -822,6 +823,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>       struct netdev_queue *txq;
>>       struct dma_desc *desc;
>>       unsigned int skb_len;
>> +     unsigned long flags;
>>       dma_addr_t mapping;
>>       u32 len_status;
>>       u16 queue;
>> @@ -831,8 +833,8 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>       txq = netdev_get_tx_queue(dev, queue);
>>       ring = &priv->tx_rings[queue];
>>
>> -     /* lock against tx reclaim in BH context */
>> -     spin_lock(&ring->lock);
>> +     /* lock against tx reclaim in BH context and TX ring full interrupt */
>> +     spin_lock_irqsave(&ring->lock, flags);
>>       if (unlikely(ring->desc_count == 0)) {
>>               netif_tx_stop_queue(txq);
>>               netdev_err(dev, "queue %d awake and ring full!\n", queue);
>> @@ -914,7 +916,7 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
>>
>>       ret = NETDEV_TX_OK;
>>  out:
>> -     spin_unlock(&ring->lock);
>> +     spin_unlock_irqrestore(&ring->lock, flags);
>>       return ret;
>>  }
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>



-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html