* net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?
@ 2023-05-17 11:08 Cambda Zhu
2023-05-17 15:58 ` Eric Dumazet
2023-05-19 8:01 ` [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state Cambda Zhu
0 siblings, 2 replies; 11+ messages in thread
From: Cambda Zhu @ 2023-05-17 11:08 UTC (permalink / raw)
To: netdev, David S. Miller, Eric Dumazet
Cc: Xuan Zhuo, Dust Li, Tony Lu, Cambda Zhu
I want to call setsockopt(TCP_MAXSEG) on a listen sock to let
all child socks have smaller MSS. And I found the child sock
MSS changed but getsockopt(TCP_MAXSEG) on the listen sock
returns 536 always.
It seems the tp->mss_cache is initialized with TCP_MSS_DEFAULT,
but getsockopt(TCP_MAXSEG) returns tp->rx_opt.user_mss only when
tp->mss_cache is 0. I don't understand the purpose of the mss_cache
check of TCP_MAXSEG. If getsockopt(TCP_MAXSEG) on listen sock makes
no sense, why does it have a branch for close/listen sock to return
user_mss? If getsockopt(TCP_MAXSEG) on listen sock is ok, why does
it check mss_cache for a listen sock?
I tried to find the commit log about TCP_MAXSEG, and found that
in commit 0c409e85f0ac ("Import 2.3.41pre2"), the mss_cache check
was added. No more detailed information found. Is this a bug or am
I misunderstanding something?
Regards,
Cambda
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?
2023-05-17 11:08 net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS? Cambda Zhu
@ 2023-05-17 15:58 ` Eric Dumazet
2023-05-17 17:37 ` Cambda Zhu
2023-05-19 8:01 ` [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state Cambda Zhu
1 sibling, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2023-05-17 15:58 UTC (permalink / raw)
To: Cambda Zhu; +Cc: netdev, David S. Miller, Xuan Zhuo, Dust Li, Tony Lu
On Wed, May 17, 2023 at 1:09 PM Cambda Zhu <cambda@linux•alibaba.com> wrote:
>
> I want to call setsockopt(TCP_MAXSEG) on a listen sock to let
> all child socks have smaller MSS. And I found the child sock
> MSS changed but getsockopt(TCP_MAXSEG) on the listen sock
> returns 536 always.
>
I think TCP_MAXSEG is not like a traditional option you can set and get later,
expecting to read back the value you set.
It is probably a bug.
Getting tp->mss_cache should have been a separate socket option, but
it is too late.
> It seems the tp->mss_cache is initialized with TCP_MSS_DEFAULT,
> but getsockopt(TCP_MAXSEG) returns tp->rx_opt.user_mss only when
> tp->mss_cache is 0. I don't understand the purpose of the mss_cache
> check of TCP_MAXSEG. If getsockopt(TCP_MAXSEG) on listen sock makes
> no sense, why does it have a branch for close/listen sock to return
> user_mss? If getsockopt(TCP_MAXSEG) on listen sock is ok, why does
> it check mss_cache for a listen sock?
>
> I tried to find the commit log about TCP_MAXSEG, and found that
> in commit 0c409e85f0ac ("Import 2.3.41pre2"), the mss_cache check
> was added. No more detailed information found. Is this a bug or am
> I misunderstanding something?
I wonder if we should simply do:
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4d6392c16b7a5a9a853c27e3a4b258d000738304..cb526257a06a6c7a4e65e710fff1770bd382ed2d
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4080,9 +4080,10 @@ int do_tcp_getsockopt(struct sock *sk, int level,
switch (optname) {
case TCP_MAXSEG:
- val = tp->mss_cache;
- if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+ if (((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
val = tp->rx_opt.user_mss;
+ else
+ val = tp->mss_cache;
if (tp->repair)
val = tp->rx_opt.mss_clamp;
break;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?
2023-05-17 15:58 ` Eric Dumazet
@ 2023-05-17 17:37 ` Cambda Zhu
0 siblings, 0 replies; 11+ messages in thread
From: Cambda Zhu @ 2023-05-17 17:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David S. Miller, Xuan Zhuo, Dust Li, Tony Lu
> On May 17, 2023, at 23:58, Eric Dumazet <edumazet@google•com> wrote:
>
> On Wed, May 17, 2023 at 1:09 PM Cambda Zhu <cambda@linux•alibaba.com> wrote:
>>
>> I want to call setsockopt(TCP_MAXSEG) on a listen sock to let
>> all child socks have smaller MSS. And I found the child sock
>> MSS changed but getsockopt(TCP_MAXSEG) on the listen sock
>> returns 536 always.
>>
>
> I think TCP_MAXSEG is not like a traditional option you can set and get later,
> expecting to read back the value you set.
>
> It is probably a bug.
>
> Getting tp->mss_cache should have been a separate socket option, but
> it is too late.
>
I understand now. It seems setting/getting TCP_MAXSEG has different meaning. :)
>
>> It seems the tp->mss_cache is initialized with TCP_MSS_DEFAULT,
>> but getsockopt(TCP_MAXSEG) returns tp->rx_opt.user_mss only when
>> tp->mss_cache is 0. I don't understand the purpose of the mss_cache
>> check of TCP_MAXSEG. If getsockopt(TCP_MAXSEG) on listen sock makes
>> no sense, why does it have a branch for close/listen sock to return
>> user_mss? If getsockopt(TCP_MAXSEG) on listen sock is ok, why does
>> it check mss_cache for a listen sock?
>>
>> I tried to find the commit log about TCP_MAXSEG, and found that
>> in commit 0c409e85f0ac ("Import 2.3.41pre2"), the mss_cache check
>> was added. No more detailed information found. Is this a bug or am
>> I misunderstanding something?
>
> I wonder if we should simply do:
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 4d6392c16b7a5a9a853c27e3a4b258d000738304..cb526257a06a6c7a4e65e710fff1770bd382ed2d
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4080,9 +4080,10 @@ int do_tcp_getsockopt(struct sock *sk, int level,
>
> switch (optname) {
> case TCP_MAXSEG:
> - val = tp->mss_cache;
> - if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> + if (((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> val = tp->rx_opt.user_mss;
> + else
> + val = tp->mss_cache;
> if (tp->repair)
> val = tp->rx_opt.mss_clamp;
> break;
This fix is a good solution for me. Will you send a patch later?
Thanks,
Cambda
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-17 11:08 net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS? Cambda Zhu
2023-05-17 15:58 ` Eric Dumazet
@ 2023-05-19 8:01 ` Cambda Zhu
2023-05-23 10:28 ` Paolo Abeni
2023-05-23 13:45 ` Paolo Abeni
1 sibling, 2 replies; 11+ messages in thread
From: Cambda Zhu @ 2023-05-19 8:01 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, Xuan Zhuo, Dust Li, Tony Lu, Cambda Zhu, Jack Yang
This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
TCP_MAXSEG of sock in other state will still return tp->mss_cache.
Signed-off-by: Cambda Zhu <cambda@linux•alibaba.com>
Reported-by: Jack Yang <mingliang@linux•alibaba.com>
---
net/ipv4/tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3d18e295bb2f..713854f1f061 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4083,7 +4083,7 @@ int do_tcp_getsockopt(struct sock *sk, int level,
switch (optname) {
case TCP_MAXSEG:
val = tp->mss_cache;
- if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+ if ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))
val = tp->rx_opt.user_mss;
if (tp->repair)
val = tp->rx_opt.mss_clamp;
--
2.16.6
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-19 8:01 ` [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state Cambda Zhu
@ 2023-05-23 10:28 ` Paolo Abeni
2023-05-23 11:52 ` Eric Dumazet
2023-05-23 13:45 ` Paolo Abeni
1 sibling, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2023-05-23 10:28 UTC (permalink / raw)
To: Cambda Zhu, netdev; +Cc: Eric Dumazet, Xuan Zhuo, Dust Li, Tony Lu, Jack Yang
On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> > This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> > CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> > since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> > TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> >
> > Signed-off-by: Cambda Zhu <cambda@linux•alibaba.com>
> > Reported-by: Jack Yang <mingliang@linux•alibaba.com>
It's a bit strange not seeing Eric being mentioned above:
https://lore.kernel.org/netdev/CANn89i+3kL9pYtkxkwxwNMzvC_w3LNUum_2=3u+UyLBmGmifHA@mail.gmail.com/#t
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-23 10:28 ` Paolo Abeni
@ 2023-05-23 11:52 ` Eric Dumazet
2023-05-24 7:10 ` Cambda Zhu
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2023-05-23 11:52 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Cambda Zhu, netdev, Xuan Zhuo, Dust Li, Tony Lu, Jack Yang
On Tue, May 23, 2023 at 12:28 PM Paolo Abeni <pabeni@redhat•com> wrote:
>
> On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> > > This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> > > CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> > > since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> > > TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> > >
> > > Signed-off-by: Cambda Zhu <cambda@linux•alibaba.com>
> > > Reported-by: Jack Yang <mingliang@linux•alibaba.com>
>
> It's a bit strange not seeing Eric being mentioned above:
>
...
Usual way of dealing with this would be to add
Suggested-by: Eric Dumazet <edumazet@google•com>
Link: https://lore.kernel.org/netdev/CANn89i+3kL9pYtkxkwxwNMzvC_w3LNUum_2=3u+UyLBmGmifHA@mail.gmail.com/#t
> https://lore.kernel.org/netdev/CANn89i+3kL9pYtkxkwxwNMzvC_w3LNUum_2=3u+UyLBmGmifHA@mail.gmail.com/#t
>
> Paolo
>
Patch SGTM, thanks.
Reviewed-by: Eric Dumazet <edumazet@google•com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-23 11:52 ` Eric Dumazet
@ 2023-05-24 7:10 ` Cambda Zhu
2023-05-24 9:30 ` Jason Xing
0 siblings, 1 reply; 11+ messages in thread
From: Cambda Zhu @ 2023-05-24 7:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Paolo Abeni, Jason Xing, netdev, Xuan Zhuo, Dust Li, Tony Lu,
Jack Yang
After I did a deep search about the TCP_MAXSEG, I found this is a bit
more complicated than I think before.
I don't know whether the TCP_MAXSEG is from BSD or not, but if the
"UNIX Network Programming" is right, getting TCP_MAXSEG returns default
MSS before connecting is as expect, that's what FreeBSD does. If we
simply remove the !val check for it, getting TCP_MAXSEG will return zero
before connecting, because tp->rx_opt.user_mss is initialized with zero
on Linux, while tp->t_maxseg is initialized with default MSS on FreeBSD.
I googled to see how it's used by developers now, and I think getting
TCP_MAXSEG should return default MSS if before connecting and user_mss
not set, for backward compatibility.
But the !val check is a bug also, and the problem is not discovered for
the first time.
https://stackoverflow.com/questions/25996741/why-getsockopt-does-not-return-the-expected-value-for-tcp-maxseg
I think it should be:
- if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+ if (tp->rx_opt.user_mss && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
With this change, getting TCP_MAXSEG will return default MSS as book
described, and return user_mss if user_mss is set and before connecting.
The tp->t_maxseg will only decrease on FreeBSD and we don't. I think
our solution is better.
I'll send a new patch later.
Regards,
Cambda
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-24 7:10 ` Cambda Zhu
@ 2023-05-24 9:30 ` Jason Xing
0 siblings, 0 replies; 11+ messages in thread
From: Jason Xing @ 2023-05-24 9:30 UTC (permalink / raw)
To: Cambda Zhu
Cc: Eric Dumazet, Paolo Abeni, netdev, Xuan Zhuo, Dust Li, Tony Lu,
Jack Yang
On Wed, May 24, 2023 at 3:10 PM Cambda Zhu <cambda@linux•alibaba.com> wrote:
>
> After I did a deep search about the TCP_MAXSEG, I found this is a bit
> more complicated than I think before.
>
> I don't know whether the TCP_MAXSEG is from BSD or not, but if the
> "UNIX Network Programming" is right, getting TCP_MAXSEG returns default
> MSS before connecting is as expect, that's what FreeBSD does. If we
> simply remove the !val check for it, getting TCP_MAXSEG will return zero
> before connecting, because tp->rx_opt.user_mss is initialized with zero
> on Linux, while tp->t_maxseg is initialized with default MSS on FreeBSD.
>
> I googled to see how it's used by developers now, and I think getting
> TCP_MAXSEG should return default MSS if before connecting and user_mss
> not set, for backward compatibility.
>
> But the !val check is a bug also, and the problem is not discovered for
> the first time.
> https://stackoverflow.com/questions/25996741/why-getsockopt-does-not-return-the-expected-value-for-tcp-maxseg
>
> I think it should be:
>
> - if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> + if (tp->rx_opt.user_mss && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
Good catch, It makes sense to me. With this modification, it could
prevent getsockopt returning zero with TCP_MAXSEG flag when the socket
stays in the close/listen state.
Thanks,
Jason
>
> With this change, getting TCP_MAXSEG will return default MSS as book
> described, and return user_mss if user_mss is set and before connecting.
> The tp->t_maxseg will only decrease on FreeBSD and we don't. I think
> our solution is better.
>
> I'll send a new patch later.
>
>
> Regards,
> Cambda
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-19 8:01 ` [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state Cambda Zhu
2023-05-23 10:28 ` Paolo Abeni
@ 2023-05-23 13:45 ` Paolo Abeni
2023-05-24 1:04 ` Cambda Zhu
1 sibling, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2023-05-23 13:45 UTC (permalink / raw)
To: Cambda Zhu, netdev; +Cc: Eric Dumazet, Xuan Zhuo, Dust Li, Tony Lu, Jack Yang
On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> TCP_MAXSEG of sock in other state will still return tp->mss_cache.
>
> Signed-off-by: Cambda Zhu <cambda@linux•alibaba.com>
> Reported-by: Jack Yang <mingliang@linux•alibaba.com>
Could you please re-submit including the Eric's tags?
Thanks!
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-23 13:45 ` Paolo Abeni
@ 2023-05-24 1:04 ` Cambda Zhu
2023-05-24 2:11 ` Jason Xing
0 siblings, 1 reply; 11+ messages in thread
From: Cambda Zhu @ 2023-05-24 1:04 UTC (permalink / raw)
To: Paolo Abeni, Eric Dumazet; +Cc: netdev, Xuan Zhuo, Dust Li, Tony Lu, Jack Yang
> On May 23, 2023, at 21:45, Paolo Abeni <pabeni@redhat•com> wrote:
>
> On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
>> This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
>> CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
>> since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
>> TCP_MAXSEG of sock in other state will still return tp->mss_cache.
>>
>> Signed-off-by: Cambda Zhu <cambda@linux•alibaba.com>
>> Reported-by: Jack Yang <mingliang@linux•alibaba.com>
>
> Could you please re-submit including the Eric's tags?
>
> Thanks!
>
> Paolo
Sorry for no Eric's tag. I don't know the 'Suggested-by' tag before,
and I'll re-submit the patch.
Should I add Eric's 'Reviewed-by' to the patch? Or this should be added
by maintainer? Sorry for my ignorance again. :)
Thanks,
Cambda
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state
2023-05-24 1:04 ` Cambda Zhu
@ 2023-05-24 2:11 ` Jason Xing
0 siblings, 0 replies; 11+ messages in thread
From: Jason Xing @ 2023-05-24 2:11 UTC (permalink / raw)
To: Cambda Zhu
Cc: Paolo Abeni, Eric Dumazet, netdev, Xuan Zhuo, Dust Li, Tony Lu,
Jack Yang
On Wed, May 24, 2023 at 9:05 AM Cambda Zhu <cambda@linux•alibaba.com> wrote:
>
>
> > On May 23, 2023, at 21:45, Paolo Abeni <pabeni@redhat•com> wrote:
> >
> > On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> >> This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> >> CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> >> since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> >> TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> >>
> >> Signed-off-by: Cambda Zhu <cambda@linux•alibaba.com>
> >> Reported-by: Jack Yang <mingliang@linux•alibaba.com>
> >
> > Could you please re-submit including the Eric's tags?
> >
> > Thanks!
> >
> > Paolo
>
> Sorry for no Eric's tag. I don't know the 'Suggested-by' tag before,
> and I'll re-submit the patch.
>
> Should I add Eric's 'Reviewed-by' to the patch? Or this should be added
> by maintainer? Sorry for my ignorance again. :)
Yes. I think the next submission should include a Reviewed-by label as
the previous email said from Eric. Oh, don't forget to include your
previous discussion link :)
Thanks,
Jason
>
> Thanks,
> Cambda
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-24 9:31 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-17 11:08 net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS? Cambda Zhu
2023-05-17 15:58 ` Eric Dumazet
2023-05-17 17:37 ` Cambda Zhu
2023-05-19 8:01 ` [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state Cambda Zhu
2023-05-23 10:28 ` Paolo Abeni
2023-05-23 11:52 ` Eric Dumazet
2023-05-24 7:10 ` Cambda Zhu
2023-05-24 9:30 ` Jason Xing
2023-05-23 13:45 ` Paolo Abeni
2023-05-24 1:04 ` Cambda Zhu
2023-05-24 2:11 ` Jason Xing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox