[bug report] mptcp: refine mptcp_cleanup_rbuf

[bug report] mptcp: refine mptcp_cleanup_rbuf

[Dan Carpenter]

Hello Paolo Abeni,

The patch fde56eea01f9: "mptcp: refine mptcp_cleanup_rbuf" from Jun
22, 2021, leads to the following static checker warning:

	net/mptcp/protocol.c:464 mptcp_subflow_could_cleanup()
	warn: masking a bool

net/mptcp/protocol.c
   455  static bool mptcp_subflow_could_cleanup(const struct sock *ssk, bool rx_empty)
   456  {
   457          const struct inet_connection_sock *icsk = inet_csk(ssk);
   458          bool ack_pending = READ_ONCE(icsk->icsk_ack.pending);
                ^^^^^^^^^^^^^^^^

   459          const struct tcp_sock *tp = tcp_sk(ssk);
   460  
   461          return (ack_pending & ICSK_ACK_SCHED) &&
                                      ^^^^^^^^^^^^^^
This is 1 so it works

   462                  ((READ_ONCE(tp->rcv_nxt) - READ_ONCE(tp->rcv_wup) >
   463                    READ_ONCE(icsk->icsk_ack.rcv_mss)) ||
   464                   (rx_empty && ack_pending &
   465                                (ICSK_ACK_PUSHED2 | ICSK_ACK_PUSHED)));
                                       ^^^^^^^^^^^^^^^^
This is 8 so it doesn't work

   466  }

regards,
dan carpenter

Re: [bug report] mptcp: refine mptcp_cleanup_rbuf

[Matthieu Baerts]

Hi Dan,

Thank you for the good bug report (as always)!

On 25/06/2021 10:26, Dan Carpenter wrote:
> The patch fde56eea01f9: "mptcp: refine mptcp_cleanup_rbuf" from Jun
> 22, 2021, leads to the following static checker warning:
> 
> 	net/mptcp/protocol.c:464 mptcp_subflow_could_cleanup()
> 	warn: masking a bool
> 
> net/mptcp/protocol.c
>    455  static bool mptcp_subflow_could_cleanup(const struct sock *ssk, bool rx_empty)
>    456  {
>    457          const struct inet_connection_sock *icsk = inet_csk(ssk);
>    458          bool ack_pending = READ_ONCE(icsk->icsk_ack.pending);
>                 ^^^^^^^^^^^^^^^^

It should be a u8 I presume, no reason to use a boolean here:

---------- 8< ----------

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c

index ddce5b7bbefd..199a36fe4f69 100644

--- a/net/mptcp/protocol.c

+++ b/net/mptcp/protocol.c

@@ -455,7 +455,7 @@ static void mptcp_subflow_cleanup_rbuf(struct sock *ssk)

 static bool mptcp_subflow_could_cleanup(const struct sock *ssk, bool
rx_empty)

 {

        const struct inet_connection_sock *icsk = inet_csk(ssk);

-       bool ack_pending = READ_ONCE(icsk->icsk_ack.pending);

+       u8 ack_pending = READ_ONCE(icsk->icsk_ack.pending);

        const struct tcp_sock *tp = tcp_sk(ssk);



        return (ack_pending & ICSK_ACK_SCHED) &&


---------- 8< ----------

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [bug report] mptcp: refine mptcp_cleanup_rbuf

[Paolo Abeni]

Hello,

On Fri, 2021-06-25 at 10:47 +0200, Matthieu Baerts wrote:
> Thank you for the good bug report (as always)!
> 
> On 25/06/2021 10:26, Dan Carpenter wrote:
> > The patch fde56eea01f9: "mptcp: refine mptcp_cleanup_rbuf" from Jun
> > 22, 2021, leads to the following static checker warning:
> > 
> > 	net/mptcp/protocol.c:464 mptcp_subflow_could_cleanup()
> > 	warn: masking a bool
> > 
> > net/mptcp/protocol.c
> >    455  static bool mptcp_subflow_could_cleanup(const struct sock *ssk, bool rx_empty)
> >    456  {
> >    457          const struct inet_connection_sock *icsk = inet_csk(ssk);
> >    458          bool ack_pending = READ_ONCE(icsk->icsk_ack.pending);
> >                 ^^^^^^^^^^^^^^^^
> 
> It should be a u8 I presume, no reason to use a boolean here:
> 
> ---------- 8< ----------
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> 
> index ddce5b7bbefd..199a36fe4f69 100644
> 
> --- a/net/mptcp/protocol.c
> 
> +++ b/net/mptcp/protocol.c
> 
> @@ -455,7 +455,7 @@ static void mptcp_subflow_cleanup_rbuf(struct sock *ssk)
> 
>  static bool mptcp_subflow_could_cleanup(const struct sock *ssk, bool
> rx_empty)
> 
>  {
> 
>         const struct inet_connection_sock *icsk = inet_csk(ssk);
> 
> -       bool ack_pending = READ_ONCE(icsk->icsk_ack.pending);
> 
> +       u8 ack_pending = READ_ONCE(icsk->icsk_ack.pending);
> 
>         const struct tcp_sock *tp = tcp_sk(ssk);
> 
> 
> 
>         return (ack_pending & ICSK_ACK_SCHED) &&
> 
> 
> ---------- 8< ----------

Thanks for the report! 

Yes, 'bool' usage is wrong (blame on me!), changing it to u8 should
fix.

@Mat: can you please go ahead with the patch?

Thanks!

/P