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
