* [PATCH net] af_packet: fix the tx skb protocol in raw sockets with ETH_P_ALL
@ 2019-03-18 5:39 Yoshiki Komachi
2019-03-18 14:49 ` Willem de Bruijn
0 siblings, 1 reply; 4+ messages in thread
From: Yoshiki Komachi @ 2019-03-18 5:39 UTC (permalink / raw)
To: David S. Miller; +Cc: Yoshiki Komachi, netdev, Yoshiki Komachi
I am using "protocol ip" filters in TC to manipulate TC flower
classifiers, which are only available with "protocol ip". However,
I faced an issue that packets sent via raw sockets with ETH_P_ALL
did not match the ip filters even if they did satisfy the condition
(e.g., DHCP offer from dhcpd).
I have determined that the behavior was caused by an unexpected
value stored in skb->protocol, namely, ETH_P_ALL instead of ETH_P_IP,
when packets were sent via raw sockets with ETH_P_ALL set.
IMHO, storing ETH_P_ALL in skb->protocol is not appropriate for
packets sent via raw sockets because ETH_P_ALL is not a real ether
type used on wire, but a virtual one.
This patch fixes the tx protocol selection in cases of transmission
via raw sockets created with ETH_P_ALL so that it asks the driver to
extract protocol from the Ethernet header.
Signed-off-by: Yoshiki Komachi <komachi.yoshiki@lab.ntt.co.jp>
---
net/packet/af_packet.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 8376bc1..9f6170f 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1852,7 +1852,8 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
{
- if (!skb->protocol && sock->type == SOCK_RAW) {
+ if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
+ sock->type == SOCK_RAW) {
skb_reset_mac_header(skb);
skb->protocol = dev_parse_header_protocol(skb);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] af_packet: fix the tx skb protocol in raw sockets with ETH_P_ALL
2019-03-18 5:39 [PATCH net] af_packet: fix the tx skb protocol in raw sockets with ETH_P_ALL Yoshiki Komachi
@ 2019-03-18 14:49 ` Willem de Bruijn
2019-03-19 1:12 ` David Miller
2019-10-17 3:02 ` Maciej Żenczykowski
0 siblings, 2 replies; 4+ messages in thread
From: Willem de Bruijn @ 2019-03-18 14:49 UTC (permalink / raw)
To: Yoshiki Komachi
Cc: David S. Miller, Network Development, Yoshiki Komachi,
Maxim Mikityanskiy
On Mon, Mar 18, 2019 at 1:41 AM Yoshiki Komachi
<komachi.yoshiki@lab.ntt.co.jp> wrote:
>
> I am using "protocol ip" filters in TC to manipulate TC flower
> classifiers, which are only available with "protocol ip". However,
> I faced an issue that packets sent via raw sockets with ETH_P_ALL
> did not match the ip filters even if they did satisfy the condition
> (e.g., DHCP offer from dhcpd).
>
> I have determined that the behavior was caused by an unexpected
> value stored in skb->protocol, namely, ETH_P_ALL instead of ETH_P_IP,
> when packets were sent via raw sockets with ETH_P_ALL set.
>
> IMHO, storing ETH_P_ALL in skb->protocol is not appropriate for
> packets sent via raw sockets because ETH_P_ALL is not a real ether
> type used on wire, but a virtual one.
>
> This patch fixes the tx protocol selection in cases of transmission
> via raw sockets created with ETH_P_ALL so that it asks the driver to
> extract protocol from the Ethernet header.
>
> Signed-off-by: Yoshiki Komachi <komachi.yoshiki@lab.ntt.co.jp>
Acked-by: Willem de Bruijn <willemb@google.com>
The Fixes tag is missing. Calling sendmsg on a packet socket bound to
ETH_P_ALL goes back a long way. It is a user, not kernel, bug to do
so.
But no more than sending on a socket bound to ETH_P_NONE (0), which
was addressed in commit c72219b75fde ("packet: infer protocol from
ethernet header if unset").
Probing with packet_parse_headers is available only as of commit
75c65772c3 ("net/packet: Ask driver for protocol if not provided by
user") in v5.1-rc1. This fix does not need to go back further than
that, imho.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] af_packet: fix the tx skb protocol in raw sockets with ETH_P_ALL
2019-03-18 14:49 ` Willem de Bruijn
@ 2019-03-19 1:12 ` David Miller
2019-10-17 3:02 ` Maciej Żenczykowski
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2019-03-19 1:12 UTC (permalink / raw)
To: willemdebruijn.kernel; +Cc: komachi.yoshiki, netdev, ysk, maximmi
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 18 Mar 2019 10:49:56 -0400
> On Mon, Mar 18, 2019 at 1:41 AM Yoshiki Komachi
> <komachi.yoshiki@lab.ntt.co.jp> wrote:
>>
>> I am using "protocol ip" filters in TC to manipulate TC flower
>> classifiers, which are only available with "protocol ip". However,
>> I faced an issue that packets sent via raw sockets with ETH_P_ALL
>> did not match the ip filters even if they did satisfy the condition
>> (e.g., DHCP offer from dhcpd).
>>
>> I have determined that the behavior was caused by an unexpected
>> value stored in skb->protocol, namely, ETH_P_ALL instead of ETH_P_IP,
>> when packets were sent via raw sockets with ETH_P_ALL set.
>>
>> IMHO, storing ETH_P_ALL in skb->protocol is not appropriate for
>> packets sent via raw sockets because ETH_P_ALL is not a real ether
>> type used on wire, but a virtual one.
>>
>> This patch fixes the tx protocol selection in cases of transmission
>> via raw sockets created with ETH_P_ALL so that it asks the driver to
>> extract protocol from the Ethernet header.
>>
>> Signed-off-by: Yoshiki Komachi <komachi.yoshiki@lab.ntt.co.jp>
>
> Acked-by: Willem de Bruijn <willemb@google.com>
Applied.
> The Fixes tag is missing. Calling sendmsg on a packet socket bound to
> ETH_P_ALL goes back a long way. It is a user, not kernel, bug to do
> so.
>
> But no more than sending on a socket bound to ETH_P_NONE (0), which
> was addressed in commit c72219b75fde ("packet: infer protocol from
> ethernet header if unset").
>
> Probing with packet_parse_headers is available only as of commit
> 75c65772c3 ("net/packet: Ask driver for protocol if not provided by
> user") in v5.1-rc1. This fix does not need to go back further than
> that, imho.
I added 75c65772c3 as the Fixes: tag, and will not queue this up for
-stable.
Thanks for the analysis.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] af_packet: fix the tx skb protocol in raw sockets with ETH_P_ALL
2019-03-18 14:49 ` Willem de Bruijn
2019-03-19 1:12 ` David Miller
@ 2019-10-17 3:02 ` Maciej Żenczykowski
1 sibling, 0 replies; 4+ messages in thread
From: Maciej Żenczykowski @ 2019-10-17 3:02 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Yoshiki Komachi, David S. Miller, Network Development,
Yoshiki Komachi, Maxim Mikityanskiy
> The Fixes tag is missing. Calling sendmsg on a packet socket bound to
> ETH_P_ALL goes back a long way. It is a user, not kernel, bug to do
> so.
I would argue it's not a bug at all - at least not the way you phrased it.
AFAIK it can/could be done correctly via specifying the protocol in
the sll_protocol field of the struct sockaddr_ll passed to sendmsg as
the target address.
Not specifying it would indeed be a bug (and a common one)
Hence I do agree it's better to automate this.
> But no more than sending on a socket bound to ETH_P_NONE (0), which
> was addressed in commit c72219b75fde ("packet: infer protocol from
> ethernet header if unset").
This is also not a bug. That's simply how you send when you don't
want to receive.
Other approaches add your socket to the receive hooks and slow the system
down (even if you add a bpf filter to unconditionally drop stuff).
ie. this is the *optimal* way to send frames.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-17 3:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-18 5:39 [PATCH net] af_packet: fix the tx skb protocol in raw sockets with ETH_P_ALL Yoshiki Komachi
2019-03-18 14:49 ` Willem de Bruijn
2019-03-19 1:12 ` David Miller
2019-10-17 3:02 ` Maciej Żenczykowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.