Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Paolo Abeni]

Whoops, I omitted the ML in my reply, sorry...

@Florian: sorry for the dups! please reply to this one, if needed, to
re-include properly the ML into the thread.

On Wed, 2021-11-10 at 09:53 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > > +	if (sk->sk_state == TCP_LISTEN)
> > > +		return -EINVAL;
> > > +
> > > +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> > > +		return 0;
> > > +
> > > +	delta = READ_ONCE(msk->write_seq) - v;
> > 
> > This is under the msk socket lock and write_seq is protected by the
> > full/plain msk socket lock so READ_ONCE should not be necessary, I
> > think. The same for 'snd_nxt' below. 
> 
> Then why is write_seq updated with WRITE_ONCE in lots of places?

'write_seq' is read outside the msk socket lock scope in a few places. 

My understanding is that to avoid reces and ubsan splat we need to mark
each write with WRITE_ONCE, and read outside the socket lock with
READ_ONCE.

If all the write happens under the socket lock, read under the socket
lock don't need the _ONCE annotation. My reference is:

https://marc.info/?l=linux-netdev&m=161198643714502&w=2

Feel free to correct me if I'm wrong! 

BTW this always reports '1' on fallback sockets after shutdown, as
after shutdown write_seq is incremented by 1, but snd_una/snd_nxt are
not incremented when receiving the TCP fin. I guess the fix not
strictly related to this patch.

Cheers,

Paolo

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> Whoops, I omitted the ML in my reply, sorry...
> 
> @Florian: sorry for the dups! please reply to this one, if needed, to
> re-include properly the ML into the thread.
> 
> On Wed, 2021-11-10 at 09:53 +0100, Florian Westphal wrote:
> > Paolo Abeni <pabeni@redhat.com> wrote:
> > > > +	if (sk->sk_state == TCP_LISTEN)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> > > > +		return 0;
> > > > +
> > > > +	delta = READ_ONCE(msk->write_seq) - v;
> > > 
> > > This is under the msk socket lock and write_seq is protected by the
> > > full/plain msk socket lock so READ_ONCE should not be necessary, I
> > > think. The same for 'snd_nxt' below. 
> > 
> > Then why is write_seq updated with WRITE_ONCE in lots of places?
> 
> 'write_seq' is read outside the msk socket lock scope in a few places. 

Bah.  You are right. TBH I do not understand READ|WRITE_ONCE usage in
mptcp anymore.  Maybe its correct, who knows.

The different locking schemes are not easy to follow for me.

> BTW this always reports '1' on fallback sockets after shutdown, as
> after shutdown write_seq is incremented by 1, but snd_una/snd_nxt are
> not incremented when receiving the TCP fin. I guess the fix not
> strictly related to this patch.

That should be ok, we should return nonzero inq hint when next read()
would indicate EOF.

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Paolo Abeni]

On Wed, 2021-11-10 at 12:37 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > Whoops, I omitted the ML in my reply, sorry...
> > 
> > @Florian: sorry for the dups! please reply to this one, if needed, to
> > re-include properly the ML into the thread.
> > 
> > On Wed, 2021-11-10 at 09:53 +0100, Florian Westphal wrote:
> > > Paolo Abeni <pabeni@redhat.com> wrote:
> > > > > +	if (sk->sk_state == TCP_LISTEN)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> > > > > +		return 0;
> > > > > +
> > > > > +	delta = READ_ONCE(msk->write_seq) - v;
> > > > 
> > > > This is under the msk socket lock and write_seq is protected by the
> > > > full/plain msk socket lock so READ_ONCE should not be necessary, I
> > > > think. The same for 'snd_nxt' below. 
> > > 
> > > Then why is write_seq updated with WRITE_ONCE in lots of places?
> > 
> > 'write_seq' is read outside the msk socket lock scope in a few places. 
> 
> Bah.  You are right. TBH I do not understand READ|WRITE_ONCE usage in
> mptcp anymore.  Maybe its correct, who knows.
> 
> The different locking schemes are not easy to follow for me.

Indeed I agreed!

I *think* (mostly guess) some _ONCE() annotation are now obsoleted/made
unnecessary by the several re-factors.

For sure we need to document clarely the locking schema (where? how?
would comments on the protectect fields suffice?) and possibly/likely
review the current annotations.

> > BTW this always reports '1' on fallback sockets after shutdown, as
> > after shutdown write_seq is incremented by 1, but snd_una/snd_nxt are
> > not incremented when receiving the TCP fin. I guess the fix not
> > strictly related to this patch.
> 
> That should be ok, we should return nonzero inq hint when next read()
> would indicate EOF.

This is for the outq ?!? I think it should be 0 after all outstanding
data is acked ?!?

Thanks!

Paolo

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> > Bah.  You are right. TBH I do not understand READ|WRITE_ONCE usage in
> > mptcp anymore.  Maybe its correct, who knows.
> > 
> > The different locking schemes are not easy to follow for me.
> 
> Indeed I agreed!
> 
> I *think* (mostly guess) some _ONCE() annotation are now obsoleted/made
> unnecessary by the several re-factors.
> 
> For sure we need to document clarely the locking schema (where? how?
> would comments on the protectect fields suffice?) and possibly/likely
> review the current annotations.

I think adding comments in the mptcp_sock struct would be a great start.

> > > BTW this always reports '1' on fallback sockets after shutdown, as
> > > after shutdown write_seq is incremented by 1, but snd_una/snd_nxt are
> > > not incremented when receiving the TCP fin. I guess the fix not
> > > strictly related to this patch.
> > 
> > That should be ok, we should return nonzero inq hint when next read()
> > would indicate EOF.
> 
> This is for the outq ?!? I think it should be 0 after all outstanding
> data is acked ?!?

Oh, I misread it as inq.

[PATCH mptcp-next 0/3] support for setsockopt(TCP_INQ)

[Florian Westphal]

This adds TCP_INQ for mptcp and extends the selftest infra.
Last patch adds the (older) ioctls for this.

Florian Westphal (3):
  mptcp: add TCP_INQ cmsg support
  selftests: mptcp: add TCP_INQ support
  mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

 net/mptcp/protocol.c                          | 85 +++++++++++++++++++
 net/mptcp/protocol.h                          |  1 +
 net/mptcp/sockopt.c                           | 37 ++++++++
 .../selftests/net/mptcp/mptcp_connect.c       | 56 +++++++++++-
 .../selftests/net/mptcp/mptcp_sockopt.sh      | 10 +--
 5 files changed, 183 insertions(+), 6 deletions(-)

-- 
2.32.0

[PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Florian Westphal]

Allows to query in-sequence data ready for read(), total bytes in
write queue and total bytes in write queue that have not yet been sent.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/mptcp/protocol.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b4263bf821ac..29b6f57b917e 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -22,6 +22,7 @@
 #endif
 #include <net/mptcp.h>
 #include <net/xfrm.h>
+#include <asm/ioctls.h>
 #include "protocol.h"
 #include "mib.h"
 
@@ -3260,6 +3261,56 @@ static int mptcp_forward_alloc_get(const struct sock *sk)
 	return sk->sk_forward_alloc + mptcp_sk(sk)->rmem_fwd_alloc;
 }
 
+static int mptcp_ioctl_outq(const struct mptcp_sock *msk, u64 v)
+{
+	const struct sock *sk = (void *)msk;
+	u64 delta;
+
+	if (sk->sk_state == TCP_LISTEN)
+		return -EINVAL;
+
+	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
+		return 0;
+
+	delta = READ_ONCE(msk->write_seq) - v;
+	if (delta > INT_MAX)
+		delta = INT_MAX;
+
+	return (int)delta;
+}
+
+static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	int answ;
+	bool slow;
+
+	switch (cmd) {
+	case SIOCINQ:
+		if (sk->sk_state == TCP_LISTEN)
+			return -EINVAL;
+
+		slow = lock_sock_fast(sk);
+		answ = mptcp_inq_hint(sk);
+		unlock_sock_fast(sk, slow);
+		break;
+	case SIOCOUTQ:
+		slow = lock_sock_fast(sk);
+		answ = mptcp_ioctl_outq(msk, READ_ONCE(msk->snd_una));
+		unlock_sock_fast(sk, slow);
+		break;
+	case SIOCOUTQNSD:
+		slow = lock_sock_fast(sk);
+		answ = mptcp_ioctl_outq(msk, READ_ONCE(msk->snd_nxt));
+		unlock_sock_fast(sk, slow);
+		break;
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return put_user(answ, (int __user *)arg);
+}
+
 static struct proto mptcp_prot = {
 	.name		= "MPTCP",
 	.owner		= THIS_MODULE,
@@ -3272,6 +3323,7 @@ static struct proto mptcp_prot = {
 	.shutdown	= mptcp_shutdown,
 	.destroy	= mptcp_destroy,
 	.sendmsg	= mptcp_sendmsg,
+	.ioctl		= mptcp_ioctl,
 	.recvmsg	= mptcp_recvmsg,
 	.release_cb	= mptcp_release_cb,
 	.hash		= mptcp_hash,
-- 
2.32.0

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Matthieu Baerts]

Hi Florian,

On 08/11/2021 11:57, Florian Westphal wrote:
> Allows to query in-sequence data ready for read(), total bytes in
> write queue and total bytes in write queue that have not yet been sent.

Thank you for looking at that!

(...)

> +static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	int answ;
> +	bool slow;

Very very small detail: we should swap the two lines.
I can do that when applying the patches if no v2 is needed.

Cheers,
Matt

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Paolo Abeni]

Hello,

On Mon, 2021-11-08 at 11:57 +0100, Florian Westphal wrote:
> Allows to query in-sequence data ready for read(), total bytes in
> write queue and total bytes in write queue that have not yet been sent.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/mptcp/protocol.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b4263bf821ac..29b6f57b917e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -22,6 +22,7 @@
>  #endif
>  #include <net/mptcp.h>
>  #include <net/xfrm.h>
> +#include <asm/ioctls.h>
>  #include "protocol.h"
>  #include "mib.h"
>  
> @@ -3260,6 +3261,56 @@ static int mptcp_forward_alloc_get(const struct sock *sk)
>  	return sk->sk_forward_alloc + mptcp_sk(sk)->rmem_fwd_alloc;
>  }
>  
> +static int mptcp_ioctl_outq(const struct mptcp_sock *msk, u64 v)
> +{
> +	const struct sock *sk = (void *)msk;
> +	u64 delta;
> +
> +	if (sk->sk_state == TCP_LISTEN)
> +		return -EINVAL;
> +
> +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> +		return 0;
> +
> +	delta = READ_ONCE(msk->write_seq) - v;

This is under the msk socket lock and write_seq is protected by the
full/plain msk socket lock so READ_ONCE should not be necessary, I
think. The same for 'snd_nxt' below. 

'snd_una' instead is updated under the msk data lock, so the read once
there should be required.

Cheers,

Paolo

Re: [PATCH mptcp-next 3/3] mptcp: add SIOCINQ, OUTQ and OUTQNSD ioctls

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> > +	if (sk->sk_state == TCP_LISTEN)
> > +		return -EINVAL;
> > +
> > +	if ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))
> > +		return 0;
> > +
> > +	delta = READ_ONCE(msk->write_seq) - v;
> 
> This is under the msk socket lock and write_seq is protected by the
> full/plain msk socket lock so READ_ONCE should not be necessary, I
> think. The same for 'snd_nxt' below. 

Then why is write_seq updated with WRITE_ONCE in lots of places?