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: Thu, 22 Jan 2015 09:17:18 -0300 From: Ezequiel Garcia <ezequiel.garcia@...e-electrons.com> To: Russell King - ARM Linux <linux@....linux.org.uk> CC: netdev@...r.kernel.org, David Miller <davem@...emloft.net>, B38611@...escale.com, fabio.estevam@...escale.com Subject: Re: [PATCH 2/2] net: mv643xx_eth: Fix highmem support in non-TSO egress path On 01/21/2015 09:11 PM, Russell King - ARM Linux wrote: > On Wed, Jan 21, 2015 at 08:34:30PM -0300, Ezequiel Garcia wrote: >> I have just realised that the non-TSO and the TSO paths must work >> simultaneously (we don't know which path an egress skb will take). >> >> So, with these patches, the unmapping is done using dma_unmap_page() which >> is only correct if the skb took the non-TSO paths. In other words, >> these fixes are wrong (although I have no idea the effect of >> using dma_unmap_page on a mapping done with dma_map_single). >> >> And the problem is that in the TSO path, the linear and the non-linear >> fragments use the same kind of descriptors, so we can't distinguish >> them in the cleanup, and can't decide if _single or _page should be used. >> >> Any ideas? > > Or, maybe, if davem would reply, we might come to the conclusion (as > I previously pointed out) that it's not a driver issue, but a netdev > core issue: > > static netdev_features_t harmonize_features(struct sk_buff *skb, > netdev_features_t features) > { > ... > if (skb->ip_summed != CHECKSUM_NONE && > !can_checksum_protocol(features, type)) { > features &= ~NETIF_F_ALL_CSUM; > } else if (illegal_highdma(skb->dev, skb)) { > features &= ~NETIF_F_SG; > } > > The problem is when the first "if" is true (as is the case with IPv6 on > mv643xx_eth.c), we clear NETIF_F_ALL_CSUM, but leave NETIF_F_SG set. > > Had that first if been false, we would've called illegal_highdma(), and > found that the skb contains some highmem fragments, but the device does > *not* have NETIF_F_HIGHDMA set, and so that second "if" would be true. > The result of that is NETIF_F_SG is cleared. > > In this case, in validate_xmit_skb(), skb_needs_linearize() would be > false for a skb with fragments, causing the skb to be linearised. I've > not completely traced the GSO path, but I'd assume that does something > similar (which I think skb_segment() handles.) > > So, I'm wondering whether the above should be: > > static netdev_features_t harmonize_features(struct sk_buff *skb, > netdev_features_t features) > { > ... > if (skb->ip_summed != CHECKSUM_NONE && > !can_checksum_protocol(features, type)) { > features &= ~NETIF_F_ALL_CSUM; > } > > if (illegal_highdma(skb->dev, skb)) { > features &= ~NETIF_F_SG; > } > > So that we get NETIF_F_SG turned off for all cases (irrespective of the > NETIF_F_ALL_CSUM test) if we see a skb with highmem and we the device > does not support highdma. > > Yes, the code above hasn't changed in functionality for a long time, but > that doesn't mean it isn't buggy, and isn't the cause of our current bug. > Hm, that's interesting. > However, it would be far better to have the drivers fixed for the sake > of performance - it's only this dma_map_page() thing that is the real > cause of the problem in these drivers. > Yes, I have just sent a v2 to fix the mv643xx_eth driver (non-TSO path). If that works, I'll see about preparing a fix for mvneta, and for both egress paths. > Looking at TSO, it seems madness that it doesn't support highmem: > > void tso_start(struct sk_buff *skb, struct tso_t *tso) > { > ... > tso->data = skb->data + hdr_len; > ... > tso->data = page_address(frag->page.p) + frag->page_offset; > > Of course, this would all be a lot easier for drivers if all drivers had > to worry about was a struct page, offset and size, rather than having to > track whether each individual mapping of a transmit packet was mapped > with dma_map_single() or dma_map_page(). > > That all said, what I really care about is the regression which basically > makes 3.18 unusable on this hardware and seeing _some_ kind of resolution > to that regression - I don't care if it doesn't quite perform, what I care > about is that the network driver doesn't oops the kernel. > Thanks for all the info! -- Ezequiel GarcĂa, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com -- 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