Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)

[Antonio Quartulli <antonio@openvpn.net>]

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.