Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Menglong Dong <menglong8.dong@gmail.com>
To:     dongli.zhang@oracle.com
Cc:     andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
        daniel@iogearbox.net, davem@davemloft.net, dsahern@gmail.com,
        edumazet@google.com, imagedong@tencent.com,
        joao.m.martins@oracle.com, joe.jin@oracle.com, kuba@kernel.org,
        linux-kernel@vger.kernel.org, mingo@redhat.com,
        netdev@vger.kernel.org, rostedt@goodmis.org
Subject: [PATCH net-next v3 4/4] net: tun: track dropped skb via kfree_skb_reason()
Date:   Fri, 25 Feb 2022 13:57:32 +0800
Message-Id: <20220225055732.1830237-1-imagedong@tencent.com>
In-Reply-To: <edddb6f9-70d1-4fcf-5630-cbdfe175e8ee@oracle.com>
References: <edddb6f9-70d1-4fcf-5630-cbdfe175e8ee@oracle.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

>Hi David,
>
>On 2/22/22 6:39 AM, David Ahern wrote:
>> On 2/21/22 9:45 PM, Dongli Zhang wrote:
>>> Hi David,
>>>
>>> On 2/21/22 7:28 PM, David Ahern wrote:
>>>> On 2/20/22 10:34 PM, Dongli Zhang wrote:
>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>>> index aa27268..bf7d8cd 100644
>>>>> --- a/drivers/net/tun.c
>>>>> +++ b/drivers/net/tun.c
>>>>> @@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>>>>  	struct netdev_queue *queue;
>>>>>  	struct tun_file *tfile;
>>>>>  	int len = skb->len;
>>>>> +	enum skb_drop_reason drop_reason;
>>>>
>>>> this function is already honoring reverse xmas tree style, so this needs
>>>> to be moved up.
>>>
>>> I will move this up to before "int txq = skb->queue_mapping;".
>>>
>>>>
[...]
>>>>
>>>
>>>
>>> While there is a diff between BPF_FILTER (here) and SOCKET_FILTER ...
>>>
>>> ... indeed the issue is: there is NO diff between BPF_FILTER (here) and
>>> DEV_FILTER (introduced by the patch).
>>>
>>>
>>> The run_ebpf_filter() is to run the bpf filter attached to the TUN device (not
>>> socket). This is similar to DEV_FILTER, which is to run a device specific filter.
>>>
>>> Initially, I would use DEV_FILTER at both locations. This makes trouble to me as
>>> there would be two places with same reason=DEV_FILTER. I will not be able to
>>> tell where the skb is dropped.
>>>
>>>
>>> I was thinking about to introduce a SKB_DROP_REASON_DEV_BPF. While I have
>>> limited experience in device specific bpf, the TUN is the only device I know
>>> that has a device specific ebpf filter (by commit aff3d70a07ff ("tun: allow to
>>> attach ebpf socket filter")). The SKB_DROP_REASON_DEV_BPF is not generic enough
>>> to be re-used by other drivers.
>>>
>>>
>>> Would you mind sharing your suggestion if I would re-use (1)
>>> SKB_DROP_REASON_DEV_FILTER or (2) introduce a new SKB_DROP_REASON_DEV_BPF, which
>>> is for sk_buff dropped by ebpf attached to device (not socket).
>>>
>>>
>>> To answer your question, the SOCKET_FILTER is for filter attached to socket, the
>>> BPF_FILTER was supposed for ebpf filter attached to device (tun->filter_prog).
>>>
>>>
>> 
>> tun/tap does have some unique filtering options. The other sets focused
>> on the core networking stack is adding a drop reason of
>> SKB_DROP_REASON_BPF_CGROUP_EGRESS for cgroup based egress filters.
>
>Thank you for the explanation!
>
>> 
>> For tun unique filters, how about using a shortened version of the ioctl
>> name used to set the filter.
>> 
>
>Although TUN is widely used in virtualization environment, it is only one of
>many drivers. I prefer to not introduce a reason that can be used only by a
>specific driver.
>
>In order to make it more generic and more re-usable (e.g., perhaps people may
>add ebpf filter to TAP driver as well), how about we create below reasons.
>
>SKB_DROP_REASON_DEV_FILTER,     /* dropped by filter attached to
>				 * or directly implemented by a
>				 * specific driver
>				 */
>SKB_DROP_REASON_BPF_DEV,	/* dropped by bpf directly
>				 * attached to a specific device,
>				 * e.g., via TUNSETFILTEREBPF
>				 */

Aren't DEV_FILTER and BPF_DEV too generic? eBPF atached to netdev can
be many kinds, such as XDP, TC, etc.

I think that use TAP_TXFILTER instaed of DEV_FILTER maybe better?
and TAP_FILTER->BPF_DEV. Make them similar to the name in
__tun_chr_ioctl() may be easier for user to understand.

>
>We already use SKB_DROP_REASON_DEV_FILTER in this patchset. We will use
>SKB_DROP_REASON_BPF_DEV for the ebpf filter attached to TUN.
>
>Thank you very much!
>
>Dongli Zhang