Re: [PATCH mptcp-next v3 1/7] mptcp: drop unused @max arg of __mptcp_setsockopt_set_val

[gang.yan@linux.dev]

July 27, 2026 at 2:37 PM, "Geliang Tang" <geliang@kernel.org mailto:geliang@kernel.org?to=%22Geliang%20Tang%22%20%3Cgeliang%40kernel.org%3E > wrote:


> 
> On Mon, 2026-07-27 at 10:28 +0800, Gang Yan wrote:
> 
> > 
> > From: Gang Yan <yangang@kylinos.cn>
> >  
> >  The @max argument is never read in the function body. Remove it and
> >  the
> >  MAX_TCP_KEEP* values passed by the TCP_KEEPIDLE/INTVL/KEEPCNT
> >  callers.
> > 
> No, we shouldn't drop this "max" argument. Instead, we need to add some
> code to check the "val" argument against the "max" argument in
> __mptcp_setsockopt_set_val(), something like:
> 
>  if (val < 1 || val > max) 
>  return -EINVAL;
> 

Hi Geliang

Thank you for the review and the suggestion to add a range check inside __mptcp_setsockopt_set_val().

But, I noticed that the underlying TCP helper functions already perform their
own parameter validation internally. 

Given that these helpers already enforce the same bounds, I would like to ask
for your opinion: Is it still necessary to add an extra check in the MPTCP layer?

Thanks
Gang


> > 
> > Signed-off-by: Gang Yan <yangang@kylinos.cn>
> >  ---
> >   net/mptcp/sockopt.c | 11 ++++-------
> >   1 file changed, 4 insertions(+), 7 deletions(-)
> >  
> >  diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> >  index fcf6feb2a9eb..9c4faa0d82ef 100644
> >  --- a/net/mptcp/sockopt.c
> >  +++ b/net/mptcp/sockopt.c
> >  @@ -632,7 +632,7 @@ static int
> >  mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
> >    return ret;
> >   }
> >   
> >  -static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, int
> >  max,
> >  +static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk,
> >          int (*set_val)(struct sock *,
> >  int),
> >          int *msk_val, int val)
> >   {
> >  @@ -876,18 +876,15 @@ static int mptcp_setsockopt_sol_tcp(struct
> >  mptcp_sock *msk, int optname,
> >    ret = __mptcp_setsockopt_sol_tcp_nodelay(msk, val);
> >    break;
> >    case TCP_KEEPIDLE:
> >  - ret = __mptcp_setsockopt_set_val(msk,
> >  MAX_TCP_KEEPIDLE,
> >  - 
> >  &tcp_sock_set_keepidle_locked,
> >  + ret = __mptcp_setsockopt_set_val(msk,
> >  &tcp_sock_set_keepidle_locked,
> >    &msk-
> > keepalive_idle, val);
> >    break;
> >    case TCP_KEEPINTVL:
> >  - ret = __mptcp_setsockopt_set_val(msk,
> >  MAX_TCP_KEEPINTVL,
> >  - 
> >  &tcp_sock_set_keepintvl,
> >  + ret = __mptcp_setsockopt_set_val(msk,
> >  &tcp_sock_set_keepintvl,
> >    &msk-
> > keepalive_intvl, val);
> >    break;
> >    case TCP_KEEPCNT:
> >  - ret = __mptcp_setsockopt_set_val(msk,
> >  MAX_TCP_KEEPCNT,
> >  - 
> >  &tcp_sock_set_keepcnt,
> >  + ret = __mptcp_setsockopt_set_val(msk,
> >  &tcp_sock_set_keepcnt,
> >    &msk-
> > keepalive_cnt,
> >    val);
> >    break;
> >
>