linux-kernel - Re: [PATCH 2.6.18-rc6 1/2] dllink driver: porting v1.19 to linux 2.6.18-rc6

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: <1157620795.14882.16.camel@laptopd505.fenrus.org>
Date:	Thu, 07 Sep 2006 11:19:55 +0200
From:	Arjan van de Ven <arjan@...radead.org>
To:	Hayim Shaul <hayim@...rtent.com>
Cc:	Jeff Garzik <jeff@...zik.org>, netdev@...r.kernel.org,
	edward_peng@...nk.com.tw, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2.6.18-rc6 1/2] dllink driver: porting v1.19 to linux
	2.6.18-rc6

> @@ -335,8 +374,9 @@
>  #endif
>  	/* Read eeprom */
>  	for (i = 0; i < 128; i++) {
> -		((u16 *) sromdata)[i] = le16_to_cpu (read_eeprom (ioaddr, i));
> +		((u16 *) sromdata)[i] = cpu_to_le16 (read_eeprom (ioaddr, i));
>  	}
> +	psrom->crc = le32_to_cpu(psrom->crc);

this looks wrong, the data comes from the hw as le, so le*_to_cpu()
sounds the right direction

> @@ -401,7 +441,7 @@
>  	int i;
>  	u16 macctrl;
>  	
> -	i = request_irq (dev->irq, &rio_interrupt, IRQF_SHARED, dev->name, dev);
> +	i = request_irq (dev->irq, &rio_interrupt, SA_SHIRQ, dev->name, dev);
>  	if (i)
>  		return i;

this is backing out a fix/conversion to the new API. Bad.


>  	
> @@ -434,9 +474,12 @@
>  	writeb (0x30, ioaddr + RxDMABurstThresh);
>  	writeb (0x30, ioaddr + RxDMAUrgentThresh);
>  	writel (0x0007ffff, ioaddr + RmonStatMask);
> +
>  	/* clear statistics */
>  	clear_stats (dev);
>  
> +	atomic_set(&np->tx_desc_lock, 0);

I'm quite scared by this naming; it suggests home-brew locking

>  	dev->trans_start = jiffies;
> +	tasklet_enable(&np->tx_tasklet);
> +	writew(DEFAULT_INTR, ioaddr + IntEnable);
> +	return;

this looks like a PCI posting bug


> -rio_free_tx (struct net_device *dev, int irq) 
> +rio_free_tx (struct net_device *dev) 
>  {
>  	struct netdev_private *np = netdev_priv(dev);
>  	int entry = np->old_tx % TX_RING_SIZE;
> -	int tx_use = 0;
>  	unsigned long flag = 0;
> +	int irq = in_interrupt();

eeeeep

> +
> +	if (atomic_read(&np->tx_desc_lock))
> +		return;
> +	atomic_inc(&np->tx_desc_lock);

and yes.. it is broken self made locking....
there is a nice race between the _read and the _inc here.


>  	
>  	if (irq)
>  		spin_lock(&np->tx_lock);
>  	else
>  		spin_lock_irqsave(&np->tx_lock, flag);

double eeeep

this is wrong to do with in_interrupt() as gating factor!
Always doing the irqsave() is fine btw



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/