[MPTCP] Re: [PATCH] mptcp: mptcp reset option

[Florian Westphal <fw at strlen.de>]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> On Mon, 22 Feb 2021, Florian Westphal wrote:
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 59ea64e5e914..e95bd8fa5e62 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1648,9 +1648,21 @@ static int mptcp_event_sub_closed(struct sk_buff *skb,
> > 				  const struct mptcp_sock *msk,
> > 				  const struct sock *ssk)
> > {
> > +	const struct mptcp_subflow_context *sf;
> > +
> > 	if (mptcp_event_put_token_and_ssk(skb, msk, ssk))
> > 		return -EMSGSIZE;
> > 
> > +	sf = mptcp_subflow_ctx(ssk);
> > +	if (sf->reset_reason == 0)
> > +		return 0;
> 
> From what I see in the RFC, the T flag can be set even if the reason code is
> 'unspecified', so I don't think MPTCP_ATTR_RESET_FLAGS should be skipped if
> reset_reason == 0.

OK.

> I would suggest removing the above two lines and always including the reason
> & flags, but if it's worthwhile to optimize out either attribute then please
> explain.

Hmpf, yes, this isn't sufficient to do what I wanted.
The intention was to ONLY set any of these attributes in the nlmsg if
the subflow had been zapped via tcprst option.

as reset_reason can be 0 in the wire format (and hacks like 'lets store
v+1' are error prone), i will add another bit to signal the field is
valid instead.

> > +/* MPTCP Reset reason codes, rfc8684 */
> > +#define MPTCP_RST_EUNSPEC	0
> > +#define MPTCP_RST_EMPTCP	1
> > +#define MPTCP_RST_ERESOURCE	2
> > +#define MPTCP_RST_EPROHIBIT	3
> > +#define MPTCP_RST_EWQ2BIG	4
> > +#define MPTCP_RST_EBADPERF	5
> > +#define MPTCP_RST_EMIDDLEBOX	6
> > +
> 
> The MSP_RST_* defines could be useful in include/uapi/linux/mptcp.h

Okay, no problem. I was reluctant to place them there but
given those values are from the rfc i think it makes sense to have this
in a uapi header.

> > +			struct mptcp_ext *mpext = skb_ext_add(skb, SKB_EXT_MPTCP);
> > +
> > +			if (mpext) {
> > +				memset(mpext, 0, sizeof(*mpext));
> > +				mpext->reset_reason = MPTCP_RST_EMPTCP;
> > +			}
> 
> This chunk of code (with slight variation in the first line) is used in 3
> places, worth adding a helper function in subflow.c?

Sure, will add one.