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
| ||
|
Date: Mon, 13 Apr 2015 17:01:02 +0200 From: Felix Fietkau <nbd@...nwrt.org> To: Rafał Miłecki <zajec5@...il.com> CC: Network Development <netdev@...r.kernel.org>, Hauke Mehrtens <hauke@...ke-m.de> Subject: Re: [PATCH v4 1/9] bgmac: simplify tx ring index handling On 2015-04-13 16:21, Rafał Miłecki wrote: >> @@ -158,13 +157,7 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac, >> skb_checksum_help(skb); >> >> nr_frags = skb_shinfo(skb)->nr_frags; >> - >> - if (ring->start <= ring->end) >> - free_slots = ring->start - ring->end + BGMAC_TX_RING_SLOTS; >> - else >> - free_slots = ring->start - ring->end; >> - >> - if (free_slots <= nr_frags + 1) { >> + if (ring->end - ring->start + nr_frags + 1 >= BGMAC_TX_RING_SLOTS) { >> bgmac_err(bgmac, "TX ring is full, queue should be stopped!\n"); >> netif_stop_queue(net_dev); >> return NETDEV_TX_BUSY; > > How is this going to work with ring->end lower than ring->start? Let's > say you have 2 empty slots at the end of ring and 2 empty slots at the > beginning. In total 4 free slots. Doing ring->end - ring->start will > give you some big negative number (depending on the ring size), won't > it? It won't, because the resulting data type is limited to 32 bit. I'm pretty sure there are other places in the kernel that rely on the same behavior. >> @@ -284,10 +276,8 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring) >> slot->skb = NULL; >> } >> >> -next: >> slot->dma_addr = 0; >> - if (++ring->start >= BGMAC_TX_RING_SLOTS) >> - ring->start = 0; >> + ring->start++; >> freed = true; >> } >> > > Do I understand correctly you're using u32 overflow here? Is this > OK/allowed in kernel to knowingly use overflows? I think so. >> diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h >> index 3ad965f..5a198d5 100644 >> --- a/drivers/net/ethernet/broadcom/bgmac.h >> +++ b/drivers/net/ethernet/broadcom/bgmac.h >> @@ -414,10 +414,10 @@ enum bgmac_dma_ring_type { >> * empty. >> */ >> struct bgmac_dma_ring { >> - u16 num_slots; >> - u16 start; >> - u16 end; >> + u32 start; >> + u32 end; >> >> + u16 num_slots; >> u16 mmio_base; >> struct bgmac_dma_desc *cpu_base; >> dma_addr_t dma_base; > > Any reason for u32 instead of u16? To avoid writes touching other fields close to it. - Felix -- 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