[PATCH net] udp: don't be set unconnected if only UDP cmsg

[PATCH net] udp: don't be set unconnected if only UDP cmsg

[Yick Xie]

If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg),
"connected" should not be set to 0. Otherwise it stops
the connected socket from using the cached route.

Signed-off-by: Yick Xie <yick.xie@gmail.com>
---
 net/ipv4/udp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index c02bf011d4a6..420905be5f30 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1123,16 +1123,17 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	if (msg->msg_controllen) {
 		err = udp_cmsg_send(sk, msg, &ipc.gso_size);
-		if (err > 0)
+		if (err > 0) {
 			err = ip_cmsg_send(sk, msg, &ipc,
 					   sk->sk_family == AF_INET6);
+			connected = 0;
+		}
 		if (unlikely(err < 0)) {
 			kfree(ipc.opt);
 			return err;
 		}
 		if (ipc.opt)
 			free = 1;
-		connected = 0;
 	}
 	if (!ipc.opt) {
 		struct ip_options_rcu *inet_opt;
-- 
2.34.1

Re: [PATCH net] udp: don't be set unconnected if only UDP cmsg

[Willem de Bruijn]

Yick Xie wrote:
> If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg),
> "connected" should not be set to 0. Otherwise it stops
> the connected socket from using the cached route.
> 
> Signed-off-by: Yick Xie <yick.xie@gmail.com>

This either needs to target net-next, or have

Fixes: 2e8de8576343 ("udp: add gso segment cmsg")

I think it can be argued either way. This situation existed from the
start, and is true for other cmsg that don't affect routing as well.

If the impact of the route lookup is significant, it couls be argued
to be a performance bug.

I steer towards net-next. In which case it would be nice to also
move the ipc.opt branch and perhaps even exclude other common cmsgs,
such as SCM_TXTIME and SCM_TIMESTAMPING.

Re: [PATCH net] udp: don't be set unconnected if only UDP cmsg

[Yick Xie]

On Mon, Apr 15, 2024 at 10:11 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Yick Xie wrote:
> > If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg),
> > "connected" should not be set to 0. Otherwise it stops
> > the connected socket from using the cached route.
> >
> > Signed-off-by: Yick Xie <yick.xie@gmail.com>
>
> This either needs to target net-next, or have
>
> Fixes: 2e8de8576343 ("udp: add gso segment cmsg")

I should have added that, sorry for the mess.

> I think it can be argued either way. This situation existed from the
> start, and is true for other cmsg that don't affect routing as well.
>
> If the impact of the route lookup is significant, it couls be argued
> to be a performance bug.

With sendmsg(), any smaller gso_size could be picked up dynamically.
Then it depends, "ip_route_output_flow()" could be as expensive as
"ip_make_skb()".

> I steer towards net-next. In which case it would be nice to also
> move the ipc.opt branch and perhaps even exclude other common cmsgs,
> such as SCM_TXTIME and SCM_TIMESTAMPING.

Both are fine. Though could it be better to take an easy backporting at first?

Re: [PATCH net] udp: don't be set unconnected if only UDP cmsg

[Willem de Bruijn]

Yick Xie wrote:
> On Mon, Apr 15, 2024 at 10:11 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Yick Xie wrote:
> > > If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg),
> > > "connected" should not be set to 0. Otherwise it stops
> > > the connected socket from using the cached route.
> > >
> > > Signed-off-by: Yick Xie <yick.xie@gmail.com>
> >
> > This either needs to target net-next, or have
> >
> > Fixes: 2e8de8576343 ("udp: add gso segment cmsg")
> 
> I should have added that, sorry for the mess.

No worries.

> > I think it can be argued either way. This situation existed from the
> > start, and is true for other cmsg that don't affect routing as well.
> >
> > If the impact of the route lookup is significant, it couls be argued
> > to be a performance bug.
> 
> With sendmsg(), any smaller gso_size could be picked up dynamically.
> Then it depends, "ip_route_output_flow()" could be as expensive as
> "ip_make_skb()".
> 
> > I steer towards net-next. In which case it would be nice to also
> > move the ipc.opt branch and perhaps even exclude other common cmsgs,
> > such as SCM_TXTIME and SCM_TIMESTAMPING.
> 
> Both are fine. Though could it be better to take an easy backporting at first?

Sounds good. Please just resend with the Fixes tag.