netdev - Re: [PATCH] Re: kernel BUG in eth_alloc_tx_desc_index at drivers/net/mv643xx

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7d01f9f00701210418q6f506d20tdc9ce10b501370d7@mail.gmail.com>
Date:	Sun, 21 Jan 2007 13:18:43 +0100
From:	"Thibaut VARENE" <T-Bone@...isc-linux.org>
To:	"Jarek Poplawski" <jarkao2@...pl>
Cc:	"Dale Farnsworth" <dale@...nsworth.org>, netdev@...r.kernel.org,
	mlachwani@...sta.com
Subject: Re: [PATCH] Re: kernel BUG in eth_alloc_tx_desc_index at drivers/net/mv643xx_eth.c:1069!

On 1/11/07, Jarek Poplawski <jarkao2@...pl> wrote:
>
> PS: alas I didn't even check compiling - I had no time to
> find all compile dependencies of this driver
> ---
>
> Signed-off-by: Jarek Poplawski <jarkao2@...pl>
> ---
>
> diff -Nurp linux-2.6.20-rc4-/drivers/net/mv643xx_eth.c linux-2.6.20-rc4/drivers/net/mv643xx_eth.c
> --- linux-2.6.20-rc4-/drivers/net/mv643xx_eth.c 2006-12-18 08:57:52.000000000 +0100
> +++ linux-2.6.20-rc4/drivers/net/mv643xx_eth.c  2007-01-11 08:55:34.000000000 +0100
> @@ -312,8 +312,8 @@ int mv643xx_eth_free_tx_descs(struct net
>         int count;
>         int released = 0;
>
> +       spin_lock_irqsave(&mp->lock, flags);
>         while (mp->tx_desc_count > 0) {
> -               spin_lock_irqsave(&mp->lock, flags);
>                 tx_index = mp->tx_used_desc_q;
>                 desc = &mp->p_tx_desc_area[tx_index];
>                 cmd_sts = desc->cmd_sts;
> @@ -348,8 +348,10 @@ int mv643xx_eth_free_tx_descs(struct net
>                         dev_kfree_skb_irq(skb);

Hmm, I think this is guaranteed not to work. In between those lines
the lock is released, while data in the mp structure is still being
accessed. It seems that this bit of code is indeed not race-safe
though, I'm gonna try to figure something.

>                 released = 1;
> +               spin_lock_irqsave(&mp->lock, flags);
>         }
>
> +       spin_unlock_irqrestore(&mp->lock, flags);
>         return released;
>  }

Ugh, this is really unclean... Taking a lock "for nothing" like that
has a perf cost.

HTH

T-Bone

-- 
Thibaut VARENE
http://www.parisc-linux.org/~varenet/
-
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