| Message ID | 20230131174601.203127-1-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] udp: Pass 2 bytes of data with UDP_GRO cmsg to user-space | expand |
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.
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.
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;
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(-)