Re: [PATCH net] udp: Pass 2 bytes of data with UDP_GRO cmsg to user-space

[Jakub Sitnicki <jakub@cloudflare.com>]

On Tue, Jan 31, 2023 at 06:57 PM +01, Eric Dumazet wrote:
> On Tue, Jan 31, 2023 at 6:46 PM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> While UDP_GRO cmsg interface lacks documentation, the selftests added in
>> commit 3327a9c46352 ("selftests: add functionals test for UDP GRO") suggest
>> that the user-space should allocate CMSG_SPACE for an u16 value and
>> interpret the returned bytes as such:
>>
>> static int recv_msg(int fd, char *buf, int len, int *gso_size)
>> {
>>         char control[CMSG_SPACE(sizeof(uint16_t))] = {0};
>>         ...
>>                         if (cmsg->cmsg_level == SOL_UDP
>>                             && cmsg->cmsg_type == UDP_GRO) {
>>                                 gsosizeptr = (uint16_t *) CMSG_DATA(cmsg);
>>                                 *gso_size = *gsosizeptr;
>>                                 break;
>>                         }
>>         ...
>> }
>>
>> Today user-space will receive 4 bytes of data with an UDP_GRO cmsg, because
>> the kernel packs an int into the cmsg data, as we can confirm with strace:
>>
>>   recvmsg(8, {msg_name=...,
>>               msg_iov=[{iov_base="\0\0..."..., iov_len=96000}],
>>               msg_iovlen=1,
>>               msg_control=[{cmsg_len=20,         <-- sizeof(cmsghdr) + 4
>>                             cmsg_level=SOL_UDP,
>>                             cmsg_type=0x68}],    <-- UDP_GRO
>>                             msg_controllen=24,
>>                             msg_flags=0}, 0) = 11200
>>
>> This means that either UDP_GRO selftests are broken on big endian, or this
>> is a programming error. Assume the latter and pass only the needed 2 bytes
>> of data with the cmsg.
>>
>> Fixing it like that has an added advantage that the cmsg becomes compatible
>> with what is expected by UDP_SEGMENT cmsg. It becomes possible to reuse the
>> cmsg when GSO packets are received on one socket and sent out of another.
>>
>> Fixes: bcd1665e3569 ("udp: add support for UDP_GRO cmsg")
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>  include/linux/udp.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/udp.h b/include/linux/udp.h
>> index a2892e151644..44bb8d699248 100644
>> --- a/include/linux/udp.h
>> +++ b/include/linux/udp.h
>> @@ -125,7 +125,7 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
>>  static inline void udp_cmsg_recv(struct msghdr *msg, struct sock *sk,
>>                                  struct sk_buff *skb)
>>  {
>> -       int gso_size;
>> +       __u16 gso_size;
>>
>>         if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
>>                 gso_size = skb_shinfo(skb)->gso_size;
>> --
>> 2.39.1
>>
>
> This would break some applications.
>
> I think the test can be fixed instead, this seems less risky.

Thanks for guidance. Will fix the test.