mbox series

[net,0/2] mptcp: some fallback fixes

Message ID cover.1602262630.git.pabeni@redhat.com
Headers show
Series mptcp: some fallback fixes | expand

Message

Paolo Abeni Oct. 9, 2020, 4:59 p.m. UTC 
pktdrill pointed-out we currently don't handle properly some
fallback scenario for MP_JOIN subflows

The first patch addresses such issue.

Patch 2/2 fixes a related pre-existing issue that is more
evident after 1/2: we could keep using for MPTCP signaling
closed subflows.

Paolo Abeni (2):
  mptcp: fix fallback for MP_JOIN subflows
  mptcp: subflows garbage collection

 net/mptcp/options.c  | 32 +++++++++++++++++++++++++-------
 net/mptcp/protocol.c | 17 +++++++++++++++++
 net/mptcp/protocol.h |  2 ++
 net/mptcp/subflow.c  | 16 ++++++++++++++--
 4 files changed, 58 insertions(+), 9 deletions(-)

Comments

Jakub Kicinski Oct. 10, 2020, 6:13 p.m. UTC | #1 
On Fri,  9 Oct 2020 18:59:59 +0200 Paolo Abeni wrote:
> pktdrill pointed-out we currently don't handle properly some
> fallback scenario for MP_JOIN subflows
> 
> The first patch addresses such issue.
> 
> Patch 2/2 fixes a related pre-existing issue that is more
> evident after 1/2: we could keep using for MPTCP signaling
> closed subflows.

Applied, thanks Paolo.

You already have a few of those in the code, but:

+	if (... &&
+	    schedule_work(&mptcp_sk(sk)->work))
+		sock_hold(sk);

isn't this a fairly questionable construct?

You take a reference for the async work to release _after_ you
scheduled the async work?
Paolo Abeni Oct. 12, 2020, 8:01 a.m. UTC | #2 
On Sat, 2020-10-10 at 11:13 -0700, Jakub Kicinski wrote:
> On Fri,  9 Oct 2020 18:59:59 +0200 Paolo Abeni wrote:
> > pktdrill pointed-out we currently don't handle properly some
> > fallback scenario for MP_JOIN subflows
> > 
> > The first patch addresses such issue.
> > 
> > Patch 2/2 fixes a related pre-existing issue that is more
> > evident after 1/2: we could keep using for MPTCP signaling
> > closed subflows.
> 
> Applied, thanks Paolo.
> 
> You already have a few of those in the code, but:
> 
> +	if (... &&
> +	    schedule_work(&mptcp_sk(sk)->work))
> +		sock_hold(sk);
> 
> isn't this a fairly questionable construct?
> 
> You take a reference for the async work to release _after_ you
> scheduled the async work? 

Thank you for reviewing! Indeed we need to add some comments there:
IIRC that chunk already raised a question in the past.

Afaics, that is safe because the caller (a subflow) held a reference to
sk and sk can't be freed in between the scheduling and the next
sock_hold(). 

We have a pending refactor, targeting the next development cycle, that
will consolidate the workqueue scheduling into an helper. We will add
some comments there to clarify the above.

Thanks,

Paolo