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, 10 Nov 2008 16:29:07 -0800
From:	"Lovich, Vitali" <vlovich@...lcomm.com>
To:	"Lovich, Vitali" <vlovich@...lcomm.com>,
	Johann Baudy <johaahn@...il.com>
CC:	Evgeniy Polyakov <zbr@...emap.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] Packet socket: mmapped IO: PACKET_TX_RING

> Also, there's no need to support both header versions in the tx path -
> since v2 should be used anyways and there's no legacy (v1 headers are
> deprecated no?), we can use v2 only in the tx path.
There's also a minor typo anyways in tpacket_fill_skb where you read tp_len.  You check for the header version, & then load the tp_len from tpacket_hdr always (this code should be removed anyways).

The assignment of -EFAULT to err write before filling the fragments is redundant since err is never used after that.

You don't reset the tp_status flag when you reject a message for being too short.

You incorrectly calculate to_write in the situation of sock->type == SOCK_DGRAM.

You don't check for the MAX_SKB_FRAGS condition.

Not your code, but in packet_set_ring, it's redundant to clear the status for all the frames to TP_STATUS_KERNEL (0) since the pages are allocated with kzalloc (pages are already zeroed).  I surrounded this with a CONFIG_DEBUG_KERNEL block.

Earlier, I think I had said that the mapped variable should be placed in packet_ring_buffer struct - I was wrong.  It's correct where it is.  I had forgotten that rx & tx are mapped contiguously and simultaneously.  There needs to be a note in the doc that the rings should be set up before mapping (i.e. cannot set up the rx or tx ring buffer first, mmap, and then try to change it). We do need still need a check for tx_pending_skb when sizing the ring buffer (in the situation where we unfreed skbs created by a call to send from Thread B, while Thread A unmaps the ring buffer and trys a resize).  Also, in tpacket_snd, we need to increment mapped before allocating the skb and test if the original value was 0 (we decrement after incrementing tx_pending_skb).  If it was, then that indicates that the user had unmapped the ring buffer, which means we should stop sending.  No need for additional locking because of the atomic relationship this sets up between tx_pending_skb and mapped.

I'll be posting my suggested patch tomorrow.
--
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

Powered by blists - more mailing lists