linux-kernel - Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

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]

Date:	Mon, 20 Nov 2006 01:15:34 +0000
From:	Alan <alan@...rguk.ukuu.org.uk>
To:	Jay Cliburn <jacliburn@...lsouth.net>
Cc:	jeff@...zik.org, shemminger@...l.org, romieu@...zoreil.com,
	csnook@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

Would be nice if it used atl_ not at_ so its less likely to cause
namespace clashes.

You have various macros for swaps that are pretty ugly - we have
cpu_to and le/be_to_cpu functions for most swapping cases and these are
generally optimised assembler (eg bswap on x86)

AT_DESC_USED/UNUSED would be better as inline functions but thats not a
serious concern.

Be careful with :1 bitfields when working with hardware - the compiler
has more than one choice about how to pack them.

The irq enable/disable use for locking on vlan appears unsafe. PCI
interrupt delivery is asynchronous which means you can get this happen

	card sends PCI interrupt
	We call irq_disable
	We take lock
	We poke bits
	We drop lock

	PCI interrupt arrives

This really does happen, typically its nasty to debug as well because you
usually only get it on PIII boards on the one in n-zillion times a
message collides and is retransmitted on the APIC bus.

skb->len is unsigned so <= 0 can be == 0. More importantly the subtraction
before the test will wrap and is completely unsafe (see at_xmit_frame)

Alan
-
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/