Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Subject: Re: [PATCH 2/2] net: tun: track dropped skb via kfree_skb_reason()
To:     David Ahern <dsahern@gmail.com>, netdev@vger.kernel.org,
        bpf@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, davem@davemloft.net, kuba@kernel.org,
        rostedt@goodmis.org, mingo@redhat.com, ast@kernel.org,
        daniel@iogearbox.net, andrii@kernel.org, imagedong@tencent.com,
        joao.m.martins@oracle.com, joe.jin@oracle.com, edumazet@google.com
References: <20220208035510.1200-1-dongli.zhang@oracle.com>
 <20220208035510.1200-3-dongli.zhang@oracle.com>
 <f1d1f29b-45e8-1ba3-bdbd-4c892b6a4e0f@gmail.com>
From:   Dongli Zhang <dongli.zhang@oracle.com>
Message-ID: <1467d007-c472-1f08-1db5-c6d2ba2b3a55@oracle.com>
Date:   Tue, 8 Feb 2022 10:08:09 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
In-Reply-To: <f1d1f29b-45e8-1ba3-bdbd-4c892b6a4e0f@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U2JubU1aUk1PYkRYNk9tWjl1TmN6ekFLeUVEWEZOQVVOcnB2bVowOFJYSmI1?=
 =?utf-8?B?Z3M5cjdFQ3VjbVR6S3ptNDR2aHgwc2hyVUt0YXpGS3dmWHd2cnk3TnF1YmU3?=
 =?utf-8?B?ZWxQNE5YRldUQ3pvTy8vcXJ3MEY0ZmlLQmI2bDFyWDFacFdMajVuSkMySk5o?=
 =?utf-8?B?M2lhN1pucWJ0SEovbGxLMnpta0JjZzF4Yjc0dHEvMTdqNWk0cE1pMzN6MFNm?=
 =?utf-8?B?RlJra0RxOXBGL1hXZ09ZMVhDY1pQQ3kxN1duRWtXa0lWOHBCaHhFWDlHMUxp?=
 =?utf-8?B?WEFoL3pxbFRWTHVsS1JmR2ZXaXJEVmVCMEVpMUlIRFZXRjVzNmFNcStGMmND?=
 =?utf-8?B?ZSszOGQ5dW92QUFWVWhEMEpKbnVUeGlXSnZmdjQ0VGdYNlJvRm9RakNTODBD?=
 =?utf-8?B?MFNBLzN6TDRLZzBjVDhBd01sOS9PT3kwRDBEalg5UDZjdksvUGg0U0x5ZTZr?=
 =?utf-8?B?R2trRVVZcVZRS1VudTlET0srdjErS2FrYmRXV0VOc25XQVVBcytOREJjUkh2?=
 =?utf-8?B?SEhiUTdVNTgwUjRCRzV4eGRDeCtjeXA4aGMxaUxWSEtEQnhQb21OZUJtV0Ru?=
 =?utf-8?B?L0hLeTRwREJsYldnbXhxdzVxb2tQSlBLdWtTcVRONm9hOUdDUE1OUTFMUWp2?=
 =?utf-8?B?cWxJc2dqdis5VldNWldOK3ZVMWdBSDhDRU5oWWpsMktIczZYUGNoWXdoSXBx?=
 =?utf-8?B?M3lNNG5CdXExY0dvNXNlSWFPbVJFVXc5bFo5a0ZqMHEwTEhtU20vNEpsRU9I?=
 =?utf-8?B?TmROVmVoaC9MQzhIc0c1azd2K1drTVNibjFHblV3M1p0VmtWNmpma1BYWTBS?=
 =?utf-8?B?cmZXd3BlZ3VJUE01K015blQ2Q2NSNXJ6Mk8veFcxTks3bEFxS0JzRzlnRVZh?=
 =?utf-8?B?ckhuQlM4R3Y1U3hTYWcxV3pEMGdRQjB3Z254UUxpRll1SDdsSTdLZGFoTVpl?=
 =?utf-8?B?RzdSK3ZmK21LcTJ0TjlqZUVxMUlhMmkvbXBNZWN2RFZHNG5SUnh4ME1LL3Jj?=
 =?utf-8?B?ZEJjbjR5VTMzazRJQjVCWGM2OXhJMlBjMVNYVHRxNE9mbjFIelpTblZmREJE?=
 =?utf-8?B?dkFtdGdYNks4WExocHRzUS8zVjBKNVdnbDQyMXdDRnpJVWV4VFoxWXR1QXkw?=
 =?utf-8?B?TmtYVDNGTnFNSHJCMmFCTTlQM2tZWW1xc2pFKzVmREs4aVJ2dnNodThkTkUw?=
 =?utf-8?B?bGJCRnRVQkVQNHJrMEE0cngrQ2o5RndabUY4U1Z1QStjUU5ydTlReHFvaVNZ?=
 =?utf-8?B?QVN1NzcvaWFZRUxJR0hCRDk2Rng5S05jRDA1bUVrbGRyUjZsT3NVeVlzRm5D?=
 =?utf-8?B?SVptS005UjZJdEFIRWI4ek53eHFqUENjdHhvR0NTRndkS1g1SHgwaE1NR1FH?=
 =?utf-8?B?dUpJajdMYUxIUElYT2hMUTQyam5MVW9FNjVHMkxxVVkvMnVKbTFmUXhwUzRX?=
 =?utf-8?B?a2gzRU1mVDR3TDZOV0R3V0o3RXp0QXRZYlVXQ281RjQ3a1Y3bU5MV2Qzdmc5?=
 =?utf-8?B?cXJMWjlVVFVzSTFtZCt2cGV0bDdLWkZCSGhaOHRyNERURG1RVjM2a0tQcDZR?=
 =?utf-8?B?QzdoV0lNWnpQamZrdFpvelEybU1LN0xpczVBazl4b2lWVkhYMEdta3VKaW9G?=
 =?utf-8?B?RU0rYzVuZlh3MWNTQVFuU1FRZGh1cXVsWCtRUDBiUDF0dFh5ajBraDVKWDhP?=
 =?utf-8?B?Y0FaRjRvalFaeTNDQVFzbk9Xa1NuRGJSMUVXbitINHo0MkIxMGdVb1BidUhh?=
 =?utf-8?B?MEZZdnFvdUFuV28wYzVyMldZWlpIQjdaT1VXWWdNVWNNQWNGVUhTRGViejV2?=
 =?utf-8?B?Y2YyWjlCeEl1ZUw3WGttU3RQNGI5YlBjTlFDZDI5OTFGdWNYQ1U1T3ZXanJn?=
 =?utf-8?B?VTVzWVNKNUlUQ1RMaEwrRTgrRTdZR2hiQUlUYTdDLzJZdlFXL2JrVkY2Yk5K?=
 =?utf-8?B?aFR5ZU1UVkF3Z3Y4SWlLczNrU1FMcHIrSzRaeTNmODJPakgra2dXVWFreGNY?=
 =?utf-8?B?aDIyUmFMS1dveFFGK0IrQW5SOWxETEFUYkk3aGlEUTVWQktnOWZrSHZkSUVM?=
 =?utf-8?B?STg5ZDYxL3FjckxFMTE5NVhtZ2hnMit0NUkrWnZla0xUM1NoUUpIcnJEa2Np?=
 =?utf-8?B?akRNdkJaTGR4SXNESUxKKzIyUGJpKzBCaThGQTR5Qm03NEQxM1FrY0pMU3pk?=
 =?utf-8?B?bFN4dkI5eTFXVGVvaWdoNjkvQnlHR3NEVnlnY3IxcVhLa2c4eFpieSswNmlr?=
 =?utf-8?B?TmRCM0U4N0VRWjFFVkpubXNmc1ZBPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f83392ea-1814-41de-dbeb-08d9eb2df8e7
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2022 18:08:13.3979
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: saRU3u2LhJit38tu2d5Du63tAaT3/TrTd9cFjsZ6AaaxmWCfd00qZiiejv73zFYr1yXTtaK8IG/9mx9SC6xedA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5144
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10252 signatures=673431
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 adultscore=0
 mlxlogscore=999 suspectscore=0 mlxscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202080106
X-Proofpoint-GUID: o9vqhEf3Ga37b99RAL0rluOezNVwP43y
X-Proofpoint-ORIG-GUID: o9vqhEf3Ga37b99RAL0rluOezNVwP43y
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi David,

On 2/7/22 9:03 PM, David Ahern wrote:
> On 2/7/22 7:55 PM, Dongli Zhang wrote:
>> The TUN can be used as vhost-net backend. E.g, the tun_net_xmit() is the
>> interface to forward the skb from TUN to vhost-net/virtio-net.
>>
>> However, there are many "goto drop" in the TUN driver. Therefore, the
>> kfree_skb_reason() is involved at each "goto drop" to help userspace
>> ftrace/ebpf to track the reason for the loss of packets.
>>
>> Cc: Joao Martins <joao.m.martins@oracle.com>
>> Cc: Joe Jin <joe.jin@oracle.com>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>  drivers/net/tun.c          | 33 +++++++++++++++++++++++++--------
>>  include/linux/skbuff.h     |  6 ++++++
>>  include/trace/events/skb.h |  6 ++++++
>>  3 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index fed85447701a..d67f2419dbb4 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;
>> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> 

I will avoid initializing here.

> 
>>  
>>  	rcu_read_lock();
>>  	tfile = rcu_dereference(tun->tfiles[txq]);
>>  
>>  	/* Drop packet if interface is not attached */
>> -	if (!tfile)
>> +	if (!tfile) {
>> +		drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED;

Initially I was using TUN_NOT_ATTACHED. I used a more generic DEV_NOT_ATTACHED
in order to re-use the reason in the future.

How about TUN specific TUN_NOT_ATTACHED, as the core issue is because the below
is not hit.

rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);

> 
> That is going to be a confusing reason code (tap device existed to get
> here) and does not really explain this error.
> 
> 
>>  		goto drop;
>> +	}
>>  
>>  	if (!rcu_dereference(tun->steering_prog))
>>  		tun_automq_xmit(tun, skb);
>> @@ -1078,19 +1081,27 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	/* Drop if the filter does not like it.
>>  	 * This is a noop if the filter is disabled.
>>  	 * Filter can be enabled only for the TAP devices. */
>> -	if (!check_filter(&tun->txflt, skb))
>> +	if (!check_filter(&tun->txflt, skb)) {
>> +		drop_reason = SKB_DROP_REASON_TAP_RUN_FILTER;
> 
> just SKB_DROP_REASON_TAP_FILTER

I will use SKB_DROP_REASON_TAP_FILTER.

> 
>>  		goto drop;
>> +	}
>>  
>>  	if (tfile->socket.sk->sk_filter &&
>> -	    sk_filter(tfile->socket.sk, skb))
>> +	    sk_filter(tfile->socket.sk, skb)) {
>> +		drop_reason = SKB_DROP_REASON_SKB_TRIM;
> 
> SKB_DROP_REASON_SOCKET_FILTER

Sorry for my mistake, I should have re-used this SKB_DROP_REASON_SOCKET_FILTER.

> 
> The remainder of your changes feels like another variant of your
> previous "function / line" reason code. You are creating new reason
> codes for every goto failure with a code based name. The reason needs to
> be the essence of the failure in a user friendly label.
> 

The remainder are:

- SKB_DROP_REASON_SKB_TRIM
- SKB_DROP_REASON_SKB_ORPHAN_FRAGS
- SKB_DROP_REASON_SKB_PULL
- SKB_DROP_REASON_DEV_DOWN
- SKB_DROP_REASON_SKB_COPY_DATA (introduced by Patch 1/2)

I tried to make them self-explaining and re-usable to other developers.

Yes, I am creating new reason codes for every goto failure with a code based
name because each function might be failed due to many reasons. In addition, I
need to avoid duplicate 'drop_reason' returned by a function in order to help
developer identify the specific line of code that the sk_buff is dropped.

Thank you very much!

Dongli Zhang