Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Subject: Re: [PATCH net-next v4 2/4] net: tap: track dropped skb via
 kfree_skb_reason()
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     netdev@vger.kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, davem@davemloft.net,
        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, dsahern@gmail.com,
        edumazet@google.com
References: <20220226084929.6417-1-dongli.zhang@oracle.com>
 <20220226084929.6417-3-dongli.zhang@oracle.com>
 <20220301184209.1f11b350@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
 <0556b706-cb4d-b0b6-ef29-443123afd71d@oracle.com>
 <20220302110300.1ac78804@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
From:   Dongli Zhang <dongli.zhang@oracle.com>
Message-ID: <92322369-abb6-be0e-ab52-1d1ebbe38a9c@oracle.com>
Date:   Wed, 2 Mar 2022 13:44:20 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
In-Reply-To: <20220302110300.1ac78804@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
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?MVd1UFQxemdRaFUybkdZY1FmUjl4M1dJWjhjU1BWNG5kWkc5TXk3T3paNEY5?=
 =?utf-8?B?U0llVVY5elIrQjg0bFo0ZTFpR1pFcHljbnhHc1RXMVhkK21FMTVVS2d2MlVY?=
 =?utf-8?B?YnA1NUU2ZnV3YlM5dU8xRDRYWDlSUDFPNkxxWmVCVm4vR3J1OUtTc1VsdUF3?=
 =?utf-8?B?dXRRZGxaeEc1cHpoNU0yNTBUTE1Rb3ZOcWVuVjg3b0xOTzZYMFUxOVI3aTZP?=
 =?utf-8?B?YXZiNmdJTzJ2WkN2T0drcTRZcUd1OVpRR205NHNMb2hLQjBBQjIxZmxZYXV2?=
 =?utf-8?B?Skg0L1ByNnh2YUV4TDJWL0JNNGpmSkZoNmhMRjFGeG04S29CSDJIaFlBZHd0?=
 =?utf-8?B?WFM2YWtUcVIvTWxIcXIzSkNVVThSWTlnNThoTHJ1L2M3azhMWDg5WmtXM2FE?=
 =?utf-8?B?ZU5NcjZHblljY1lDbFNldWE3YmNrdFN1QnRwUHV4a1NvYjl0ajQzSWFpcGdP?=
 =?utf-8?B?eWlTZWJ4aERFeWUwQW9KWDF4RDZJR0lGbXlUQzhiMWxNVG5XeElMcUlnUUFh?=
 =?utf-8?B?UHFWVGVyNXRKODZSYmcyMnJJWkRvT3pjT25YQzFUNnF0Slk5d1RONmRMRHRU?=
 =?utf-8?B?U3VVSFU2bVViaE1oR3lIOVJMd3lUSHllMDRabWZGb1dIV2NmSHFnb0lqNUdF?=
 =?utf-8?B?VlU1TEd5WmdhOCt3VVZkRlExMkpqcnRkUTlsTm42ZUQ0U0lvOXFWaW8xNXU2?=
 =?utf-8?B?alF2bndhajI4ZC92QVJjalRTTVMrakJXaFd3WFBCaUFvM3lPQk9wY3hKTW9Z?=
 =?utf-8?B?Nm9DQ2VUdElLdCttbEU4YVZDTEIzQmRMVFpydnU4ZWI2R3lqclhZTkpWTUxh?=
 =?utf-8?B?SUtFMEVEd1BPN21adVk1bkpQWTdWd3VHdjhDZElZRkFUWXcxNzk2MFFOQ05z?=
 =?utf-8?B?OVZyRGh1STlVSXA3MnJ0M0pBdk5tQ01LakJqSFlNbVREcmxjQkVhQkwyYXR1?=
 =?utf-8?B?OGRWS25jckF2SXRQUzdQcWw3bWR6eGxsRFVIT1pRVy9UbzRad0M1TWpoK01p?=
 =?utf-8?B?cm5kbVNhRUpmNHY1MmY2endORVVJdWZLRTBDWEg3NjEzaUNIQTZYZDk1WXFr?=
 =?utf-8?B?VnFHNGlEeGU4dFpINmh5Y3Q2R25IV2NsbThKMjdINWhBTGUrME8wSEdWei8z?=
 =?utf-8?B?TGMzdUY3YzIyR2E1UEsyR0NERmhVT1F5b05nTUlCK2R2TXkya1pJRUpVZU5t?=
 =?utf-8?B?UGRUYytvTmhXS21Wclc5Qjc1NWxrWkI2djFvb2lyQU51RmlqUm5DVWtIZGpy?=
 =?utf-8?B?TFF4eUcraHJkYUthNUNydHRyaGc4elQ1QklZNVNIRDdPSlJpd1Bqd2RPMXdY?=
 =?utf-8?B?MkNxYVR1SzYySDV3dzQ1R2dIY3JmYXBzQ3FEamt6Tjh0U0pLNVlLd3M3Wll6?=
 =?utf-8?B?L1Z6Y2o4aktyNlZnVE5EdzhlMDJYTzc3anhkQ3ZqN1d3OXUyMm9IK2VWdjQz?=
 =?utf-8?B?TjhmMGNucU1WMXpiRDBsQTlodGZLTzRmV3FGZjVZc0ovc1FNZUpjVGJjb29V?=
 =?utf-8?B?RXBCcnhVMXBIazhUY2k4ai9xTkxWckpmeTUrRnNldU4wck5pYjA4VEtkUFlZ?=
 =?utf-8?B?TGVVUmovYUU3eTJsRFpTTVpXdWdPVkV5UlRSbVdFWXYxc0ZZWjA1RjlRbS93?=
 =?utf-8?B?dE4xbGFFbm1Sck9EM1J4OUNOWE9WeFlMZ1BnZmVobkRhMXdRc1VzVzZ5dzVu?=
 =?utf-8?B?bFpNVDUydEJaOE9NZFhqbCtJVzJ3MG1Yc2VTV0lDN2ZJbGVlalQ1cTJMRm1o?=
 =?utf-8?B?VXgxTFg4S0RYRkdtL1lzYVAyQlJtUmp6aEpqdlpibDhMaVl4UHJGcWtraEEx?=
 =?utf-8?B?K1BSRmF6aW14NFlLZWY3cXVEa043STdBcHZtWk1TZlByVUFSU21WdkE5UVFs?=
 =?utf-8?B?d3JpZnF2QWxyQi9MS2YwUDFLYThLeUNnS2FTeTV3YkxrSGJEODZjZTVBQ2xD?=
 =?utf-8?B?L09hTHdOTWhxSVdhaDRCWEFRRXM1VjlTeWo1ZEk1a25CVG9mOG15QnY4elEz?=
 =?utf-8?B?ZVh5Rjl1MHN1Z0dVUjM0bzNQTXhndVhWYy9RUko5S3RtQXVaMmVOc2xMYVB1?=
 =?utf-8?B?WCt2YVR0KzJGS1lQbDdVeGRXNWNRYkp3Z1d6MVdIWXo2SG14OWYvZDIwOGxQ?=
 =?utf-8?B?RVhoL3d5c0JaWDdmMTE3V2ZXOE1JWXVFS3NBQVVSaUJXME5xSUZqSmliOUQr?=
 =?utf-8?Q?XuIFcOez49sn4MDFnInKvdOius26nDlMMDbBoHTf7bRI?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 58d1841d-9440-4df0-34ce-08d9fc95d154
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2022 21:44:24.4262
 (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: HsmwEv2EYhGlg74b38nRMrZKphjfQBN190wX6pfGNU3WcHcDREv457d572yUnyLqoFw16axI92PBwIwjg01Nrg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1677
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10274 signatures=686787
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 phishscore=0 suspectscore=0 adultscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2203020090
X-Proofpoint-GUID: P4-3B6yEJLPD5CJT7V1HLy7y-zs8_jM8
X-Proofpoint-ORIG-GUID: P4-3B6yEJLPD5CJT7V1HLy7y-zs8_jM8
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 Jakub,

On 3/2/22 11:03 AM, Jakub Kicinski wrote:
> On Wed, 2 Mar 2022 09:43:29 -0800 Dongli Zhang wrote:
>> On 3/1/22 6:42 PM, Jakub Kicinski wrote:
>>> On Sat, 26 Feb 2022 00:49:27 -0800 Dongli Zhang wrote:  
>>>> +	SKB_DROP_REASON_SKB_CSUM,	/* sk_buff checksum error */  
>>>
>>> Can we spell it out a little more? It sounds like the checksum was
>>> incorrect. Will it be clear that computing the checksum failed, rather
>>> than checksum validation failed?  
>>
>> I am just trying to make the reasons as generic as possible so that:
>>
>> 1. We may minimize the number of reasons.
>>
>> 2. People may re-use the same reason for all CSUM related issue.
> 
> The generic nature is fine, my concern is to clearly differentiate
> errors in _validating_ the checksum from errors in _generating_ them.
> "sk_buff checksum error" does not explain which one had taken place.

This is for skb_checksum_help() and it is for csum computation.

Therefore, I will keep SKB_DROP_REASON_SKB_CSUM and add 'computation' or
'generating' to the comments.

> 
>>>> +	SKB_DROP_REASON_SKB_COPY_DATA,	/* failed to copy data from or to
>>>> +					 * sk_buff
>>>> +					 */  
>>>
>>> Here should we specify that it's copying from user space?  
>>
>> Same as above. I am minimizing the number of reasons so that any memory copy for
>> sk_buff may re-use this reason.
> 
> IIUC this failure is equivalent to user passing an invalid buffer. 
> I mean something like:
> 
> 	send(fd, (void *)random(), 1000, 0);
> 
> I'd be tempted to call the reason something link SKB_UCOPY_FAULT.
> To indicate it's a problem copying from user space. EFAULT is the
> typical errno for that. WDYT?
> 

So far the reason is used for below functions' return value:

- tap_get_user() -> zerocopy_sg_from_iter()
- tap_get_user() -> skb_copy_datagram_from_iter()
- tun_net_xmit() -> skb_orphan_frags_rx() -> skb_copy_ubufs()

I will switch to SKB_UCOPY_FAULT.

Thank you very much!

Dongli Zhang