| Message ID | 1348151149.31352.109.camel@edumazet-glaptop (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, 2012-09-20 at 16:25 +0200, Eric Dumazet wrote: > Please remove this hack and try the following bugfix in raw handler > > icmp_filter() should not modify skb, or else its caller should not > assume ip_hdr() is unchanged. Did not help :(
On Thu, 2012-09-20 at 17:37 +0300, Artem Bityutskiy wrote: > On Thu, 2012-09-20 at 16:25 +0200, Eric Dumazet wrote: > > Please remove this hack and try the following bugfix in raw handler > > > > icmp_filter() should not modify skb, or else its caller should not > > assume ip_hdr() is unchanged. > > Did not help :( > What gives (after some failures) cat /proc/net/raw -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 16:56 +0200, Eric Dumazet wrote: > On Thu, 2012-09-20 at 17:37 +0300, Artem Bityutskiy wrote: > > On Thu, 2012-09-20 at 16:25 +0200, Eric Dumazet wrote: > > > Please remove this hack and try the following bugfix in raw handler > > > > > > icmp_filter() should not modify skb, or else its caller should not > > > assume ip_hdr() is unchanged. > > > > Did not help :( > > > > What gives (after some failures) > > cat /proc/net/raw Note I think the failing packets are dhcp packets, and connman seems to use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it to the right device. I'd be quite surprised though if UDP code had issues with paged Rx?? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 16:56 +0200, Eric Dumazet wrote: > On Thu, 2012-09-20 at 17:37 +0300, Artem Bityutskiy wrote: > > On Thu, 2012-09-20 at 16:25 +0200, Eric Dumazet wrote: > > > Please remove this hack and try the following bugfix in raw handler > > > > > > icmp_filter() should not modify skb, or else its caller should not > > > assume ip_hdr() is unchanged. > > > > Did not help :( > > > > What gives (after some failures) > > cat /proc/net/raw -sh-4.1# cat /proc/net/raw sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode ref pointer drops
On Thu, 2012-09-20 at 17:05 +0200, Johannes Berg wrote: > On Thu, 2012-09-20 at 16:56 +0200, Eric Dumazet wrote: > > On Thu, 2012-09-20 at 17:37 +0300, Artem Bityutskiy wrote: > > > On Thu, 2012-09-20 at 16:25 +0200, Eric Dumazet wrote: > > > > Please remove this hack and try the following bugfix in raw handler > > > > > > > > icmp_filter() should not modify skb, or else its caller should not > > > > assume ip_hdr() is unchanged. > > > > > > Did not help :( > > > > > > > What gives (after some failures) > > > > cat /proc/net/raw > > Note I think the failing packets are dhcp packets, and connman seems to > use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it > to the right device. I'd be quite surprised though if UDP code had > issues with paged Rx?? > > johannes > You told me : socket(PF_PACKET, SOCK_RAW, htons(ETH_P_EAPOL)) So I looked at raw code. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 17:05 +0200, Johannes Berg wrote: > Note I think the failing packets are dhcp packets, and connman seems to > use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it > to the right device. I'd be quite surprised though if UDP code had > issues with paged Rx?? > It could be a checksum issue ? "netstat -s" could give a clue... -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 17:18 +0200, Eric Dumazet wrote: > > Note I think the failing packets are dhcp packets, and connman seems to > > use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it > > to the right device. I'd be quite surprised though if UDP code had > > issues with paged Rx?? > You told me : socket(PF_PACKET, SOCK_RAW, htons(ETH_P_EAPOL)) > > So I looked at raw code. and even found an issue. Artem wasn't really sure, but looking at his information again it seems that the EAPOL packets (those on the raw sockets) do go through, and then DHCP fails. In the printk he did for me, I can see that the EAPOL packets are small enough to get pulled in completely in iwlwifi, and the bigger DHCP packets (~600 bytes) only go through if we linearize them. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 17:19 +0200, Eric Dumazet wrote: > On Thu, 2012-09-20 at 17:05 +0200, Johannes Berg wrote: > > > Note I think the failing packets are dhcp packets, and connman seems to > > use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it > > to the right device. I'd be quite surprised though if UDP code had > > issues with paged Rx?? > > > > It could be a checksum issue ? Hmm, would paged RX make a difference there? But I guess it could be, our packets are all CHECKSUM_NONE. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 17:22 +0200, Johannes Berg wrote: > On Thu, 2012-09-20 at 17:18 +0200, Eric Dumazet wrote: > > > > Note I think the failing packets are dhcp packets, and connman seems to > > > use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it > > > to the right device. I'd be quite surprised though if UDP code had > > > issues with paged Rx?? > > > You told me : socket(PF_PACKET, SOCK_RAW, htons(ETH_P_EAPOL)) > > > > So I looked at raw code. > > and even found an issue. Artem wasn't really sure, but looking at his > information again it seems that the EAPOL packets (those on the raw > sockets) do go through, and then DHCP fails. In the printk he did for > me, I can see that the EAPOL packets are small enough to get pulled in > completely in iwlwifi, and the bigger DHCP packets (~600 bytes) only go > through if we linearize them. OK, its a bug in UDP, I'll send a patch asap. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 17:27 +0200, Eric Dumazet wrote: > On Thu, 2012-09-20 at 17:22 +0200, Johannes Berg wrote: > > On Thu, 2012-09-20 at 17:18 +0200, Eric Dumazet wrote: > > > > > > Note I think the failing packets are dhcp packets, and connman seems to > > > > use an AF_INET, SOCK_DGRAM, IPPROTO_UDP socket for those, and binds it > > > > to the right device. I'd be quite surprised though if UDP code had > > > > issues with paged Rx?? > > > > > You told me : socket(PF_PACKET, SOCK_RAW, htons(ETH_P_EAPOL)) > > > > > > So I looked at raw code. > > > > and even found an issue. Artem wasn't really sure, but looking at his > > information again it seems that the EAPOL packets (those on the raw > > sockets) do go through, and then DHCP fails. In the printk he did for > > me, I can see that the EAPOL packets are small enough to get pulled in > > completely in iwlwifi, and the bigger DHCP packets (~600 bytes) only go > > through if we linearize them. > > OK, its a bug in UDP, I'll send a patch asap. Oh well, false alarm. Artem, could you send one capture of one such DHCP big packet ? tcpdump -p -n -s 2000 -i wlanX -X -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-20 at 17:31 +0200, Eric Dumazet wrote: > Artem, could you send one capture of one such DHCP big packet ? > > tcpdump -p -n -s 2000 -i wlanX -X > You also can add the -v flag -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 20 Sep 2012 16:25:49 +0200 > Please remove this hack and try the following bugfix in raw handler > > icmp_filter() should not modify skb, or else its caller should not > assume ip_hdr() is unchanged. Right, good catch. Please submit this fix formally Eric, thanks a lot. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c index f242578..3fa8c96 100644 --- a/net/ipv4/raw.c +++ b/net/ipv4/raw.c @@ -128,25 +128,30 @@ found: } /* - * 0 - deliver - * 1 - block + * false - deliver + * true - block */ -static __inline__ int icmp_filter(struct sock *sk, struct sk_buff *skb) +static bool icmp_filter(struct sock *sk, const struct sk_buff *skb) { - int type; - - if (!pskb_may_pull(skb, sizeof(struct icmphdr))) - return 1; - - type = icmp_hdr(skb)->type; - if (type < 32) { + __u8 _type; + const __u8 *type; + + type = skb_header_pointer(skb, + skb_transport_offset(skb) + + offsetof(struct icmphdr, type), + sizeof(_type), + &_type); + if (!type) + return true; + + if (*type < 32) { __u32 data = raw_sk(sk)->filter.data; - return ((1 << type) & data) != 0; + return ((1U << *type) & data) != 0; } /* Do not block unknown ICMP types */ - return 0; + return false; } /* IP input processing comes here for RAW socket delivery.