[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e767dc62-85bc-4e80-aed9-b2957cd37c78@openvpn.net>
Date: Thu, 21 Nov 2024 22:39:54 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 21/11/2024 01:29, Sergey Ryazanov wrote:
> On 15.11.2024 16:39, Antonio Quartulli wrote:
>> On 11/11/2024 00:54, Sergey Ryazanov wrote:
>>> Another one forgotten question, sorry about this. Please find the
>>> question inlined.
>>>
>>> On 29.10.2024 12:47, Antonio Quartulli wrote:
>>>> /* Send user data to the network
>>>> */
>>>> netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device
>>>> *dev)
>>>> {
>>>> + struct ovpn_struct *ovpn = netdev_priv(dev);
>>>> + struct sk_buff *segments, *curr, *next;
>>>> + struct sk_buff_head skb_list;
>>>> + __be16 proto;
>>>> + int ret;
>>>> +
>>>> + /* reset netfilter state */
>>>> + nf_reset_ct(skb);
>>>> +
>>>> + /* verify IP header size in network packet */
>>>> + proto = ovpn_ip_check_protocol(skb);
>>>> + if (unlikely(!proto || skb->protocol != proto)) {
>>>> + net_err_ratelimited("%s: dropping malformed payload packet\n",
>>>> + dev->name);
>>>> + dev_core_stats_tx_dropped_inc(ovpn->dev);
>>>> + goto drop;
>>>> + }
>>>
>>> The above check implies that kernel can feed a network device with
>>> skb- >protocol value mismatches actual skb content. Can you share
>>> any example of such case?
>>>
>>> If you just want to be sure that the user packet is either IPv4 or
>>> IPv6 then it can be done like this and without error messages:
>>>
>>> /* Support only IPv4 or IPv6 traffic transporting */
>>> if (unlikely(skb->protocol == ETH_P_IP || skb->protocol == ETH_P_IPV6))
>>> goto drop;
>>
>> It look good, but I will still increase the drop counter, because
>> something entered the interface and we are trashing it.
>
> Sure. I just shared a minimalistic example and don't mind if the case
> will be counted. Just a small hint, the counter can be moved to the
> 'drop:' label below.
ok, will double check. thanks
>
>
> And sorry for misguiding, the '->protocol' field value has network
> endians, so constants should be wrapped in htons():
>
> if (unlikely(skb->protocol == htons(ETH_P_IP) ||
> skb->protocol == htons(ETH_P_IPV6)))
yap yap, already considered. thanks for pointing it out though.
> goto drop;
>
>> Why not printing a message? The interface is not Ethernet based, so I
>> think we should not expect anything else other than v4 or v6, no?
>
> Non-Ethernet encapsulation doesn't give any guaranty that packets will
> be IPv4/IPv6 only. There are 65k possible 'protocols' and this is an
> interface function, which technically can be called with any protocol type.
>
> With this given, nobody wants to flood the log with messages for every
> MPLS/LLDP/etc packet. Especially with messages saying that the packet is
> malformed and giving no clue, why the packet was considered wrong.
Ok, I see. I am dropping the message then.
Regards,
>
> --
> Sergey
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists