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

[Florian Westphal <fw@strlen.de>]

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.