Re: [PATCH net-next 2/2] tun: allow to attach ebpf socket filter

[Willem de Bruijn]

>>>   /* Net device start xmit */
>>>   static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device
>>> *dev)
>>>   {
>>>          struct tun_struct *tun = netdev_priv(dev);
>>>          int txq = skb->queue_mapping;
>>>          struct tun_file *tfile;
>>> +       int len = skb->len;
>>>
>>>          rcu_read_lock();
>>>          tfile = rcu_dereference(tun->tfiles[txq]);
>>> @@ -1015,9 +1029,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff
>>> *skb, struct net_device *dev)
>>>              sk_filter(tfile->socket.sk, skb))
>>>                  goto drop;
>>>
>>> +       len = run_ebpf_filter(tun, skb, len);
>>> +       if (!len)
>>> +               goto drop;
>>> +
>>
>> This adds a second filter step independent of the sk_filter call above.
>> Perhaps the two filter interfaces can map onto to the same instance.
>> I imagine that qemu never programs SO_ATTACH_FILTER.
>
>
> I think you mean TUNATTACHFILTER here (and we could not assume the tun is
> only used by qemu). A quick glance does not give any idea on how to reuse it
> for eBPF or differ eBPF from cBPF.
>
> Btw, there're other differences. TUNATTACHBPF attach the prog to tun which
> means it simplifies lots of things e.g persist devices or queue
> enabling/disabling.  But TUNATTACHFILTER attach the prog to socket.

Sounds good. Thanks for taking a look whether it could be easily
deduplicated.

>>
>> More importantly, should this program just return a boolean pass or
>> drop. Taking a length and trimming may introduce bugs later on if the
>> stack parses the packet unconditionally, expecting a minimum size
>> to be present.
>>
>> This was the reason for introducing sk_filter_trim_cap and using that
>> in other sk_filter sites.
>>
>> A quick scan shows that tun_put_user expects a full vlan tag to exist
>> if skb_vlan_tag_present(skb), for instance. If trimmed to below this
>> length the final call to skb_copy_datagram_iter may have negative
>> length.
>>
>> This is an issue with the existing sk_filter call as much as with the
>> new run_ebpf_filter call.
>
>
> Good point, so consider it was used by sk_filter too, we need to fix it
> anyway. Actually, I've considered the boolean return value but finally I
> decide to obey the style of sk filter. Maybe the trimming has real user. e.g
> high speed header recoding/analysis? Consider it's not hard to fix, how
> about just keep that?

I don't see an obvious use case, but sure. We'll just need to look
at what the minimum trim length needs to be.