netdev - Re: [RFC PATCH] af_packet: don't to defrag shared skb

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: <1354915824.9124.11.camel@jlt4.sipsolutions.net>
Date:	Fri, 07 Dec 2012 22:30:24 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	David Miller <davem@...emloft.net>
Cc:	eric@...it.org, netdev@...r.kernel.org,
	linux-wireless@...r.kernel.org, linville@...driver.com
Subject: Re: [RFC PATCH] af_packet: don't to defrag shared skb

> Wireless folks, please take a look.  The issue is that,
> under the circumstances listed below, we get SKBs in
> the AF_PACKET input path that are shared.

Ok so I took a look, but I can't see where the wireless stack is going
wrong.

> Given the logic present in ieee80211_deliver_skb() I think
> the mac80211 code doesn't expect this either.

This is correct, but the driver should never give us a shared skb. From
the other mail it seems Eric is using iwlwifi, which is definitely not
creating shared SKBs. Nothing in mac80211 creates them either.

> > I've observed this crash under the following condition:
> >  1. a program is listening to an wifi interface (let say wlan0)
> >  2. it is using fanout capture in flow load balancing mode
> >  3. defrag option is on on the fanout socket

How do you set this up, and what does it do? I'd like to try to
reproduce this.

> >  4. the interface disconnect (radio down for example)
> >  5. the interface reconnect (radio switched up)
> >  6. once reconnected a single packet is seen with skb->users=2

That's interesting. A single one seems odd. I might have expected two,
but not one. Well, since you removed the crash ... I guess I'll have to
believe that there's just one and the second one doesn't show up because
we crashed before :-)

> So if we look at ieee80211_deliver_skb(), it has code to deal with unaligned
> packet headers, wherein it memoves() the data into a better aligned location.
> 
> But if these SKBs really are skb_shared(), this packet data
> modification is illegal.
> 
> I suspect that the assumptions built into this unaligned data handling
> code, and AF_PACKET, are correct.  Meaning that we should never see
> skb_shared() packets here.  We just have a missing skb_copy()
> somewhere in mac80211, Johannes can you please take a look?

My first theory was related to multiple virtual interfaces, but Eric
didn't say he was running that, but we use skb_copy() for that in
ieee80211_prepare_and_rx_handle(). That's not necessarily the most
efficient (another reason for drivers to use paged RX here) but clearly
not causing the issue.

The only other theory I can come up with right now is that the skb_get()
happens in deliver_skb via __netif_receive_skb. Keeping in mind that
wpa_supplicant might have another packet socket open for authentication
packets, that seems like a possibility. I'll test it once I figure out
how to do this "defrag" option you speak of :)

johannes

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