* [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support
@ 2021-01-20 17:19 Florian Westphal
0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2021-01-20 17:19 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 237 bytes --]
Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> > + sk_err = ssk->sk_err;
>
> Could we have an error set on the msk? (sk->sk_err)
Userspace interacts with the msk so it should already
get proper errnos via syscalls, no?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support
@ 2021-01-26 16:51 Mat Martineau
0 siblings, 0 replies; 6+ messages in thread
From: Mat Martineau @ 2021-01-26 16:51 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2693 bytes --]
On Fri, 22 Jan 2021, Matthieu Baerts wrote:
> Hi Florian,
>
> On 20/01/2021 18:15, Matthieu Baerts wrote:
>> On 20/01/2021 16:43, Florian Westphal wrote:
>>
>> > diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
>> > index 3674a451a18c..4983ced8e1f8 100644
>> > --- a/include/uapi/linux/mptcp.h
>> > +++ b/include/uapi/linux/mptcp.h
>> > @@ -36,6 +36,7 @@ enum {
>> > /* netlink interface */
>> > #define MPTCP_PM_NAME "mptcp_pm"
>> > #define MPTCP_PM_CMD_GRP_NAME "mptcp_pm_cmds"
>> > +#define MPTCP_PM_EV_GRP_NAME "mptcp_events"
>>
>> We will check if it is OK for Ossama :)
>
> Just to be sure it is clear for everyone, it was decided yesterday at the
> weekly meeting to switch to "mptcp_pm_events".
>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index e2f40985b450..c2939362f47a 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>
>> (...)
>>
>>> +static int mptcp_event_put_token_and_ssk(struct sk_buff *skb,
>>> + const struct mptcp_sock *msk,
>>> + const struct sock *ssk)
>>> +{
>>> + const struct sock *sk = (const struct sock *)msk;
>>> + const struct mptcp_subflow_context *sf;
>>> + u8 sk_err;
>>> +
>>> + if (nla_put_u32(skb, MPTCP_ATTR_TOKEN, msk->token))
>>> + return -EMSGSIZE;
>>> +
>>> + if (mptcp_event_add_subflow(skb, ssk))
>>> + return -EMSGSIZE;
>>> +
>>> + sf = mptcp_subflow_ctx(ssk);
>>> + if (WARN_ON_ONCE(!sf))
>>> + return -EINVAL;
>>> +
>>> + if (nla_put_u8(skb, MPTCP_ATTR_BACKUP, sf->backup))
>>> + return -EMSGSIZE;
>>> +
>>> + if (ssk->sk_bound_dev_if &&
>>> + nla_put_s32(skb, MPTCP_ATTR_IF_IDX, ssk->sk_bound_dev_if))
>>> + return -EMSGSIZE;
>>> +
>>> + sk_err = ssk->sk_err;
>>
>> Could we have an error set on the msk? (sk->sk_err)
>
> And here, as discussed yesterday, we would keep "ssk->sk_err" and not use the
> one from the msk.
>
> I guess Mat and Ossama prefer to quickly validate it before applying these
> patches, I can wait before applying them. Or maybe I misunderstood and I can
> apply it sooner.
> (And if we only need to change the group name, I can fix that when applying
> it without having to submit a new version ;-) )
>
Hi Matthieu -
Go ahead and apply the series to the export branch with the group name
fix, if other fixes are needed after testing we can squash those.
Thanks,
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 6+ messages in thread* [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support
@ 2021-01-26 15:04 Florian Westphal
0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2021-01-26 15:04 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 425 bytes --]
Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> I guess Mat and Ossama prefer to quickly validate it before applying these
> patches, I can wait before applying them. Or maybe I misunderstood and I can
> apply it sooner.
> (And if we only need to change the group name, I can fix that when applying
> it without having to submit a new version ;-) )
Thanks, let me know if there are rejects and I can repost.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support
@ 2021-01-22 10:55 Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2021-01-22 10:55 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2539 bytes --]
Hi Florian,
On 20/01/2021 18:15, Matthieu Baerts wrote:
> On 20/01/2021 16:43, Florian Westphal wrote:
>
> > diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> > index 3674a451a18c..4983ced8e1f8 100644
> > --- a/include/uapi/linux/mptcp.h
> > +++ b/include/uapi/linux/mptcp.h
> > @@ -36,6 +36,7 @@ enum {
> > /* netlink interface */
> > #define MPTCP_PM_NAME "mptcp_pm"
> > #define MPTCP_PM_CMD_GRP_NAME "mptcp_pm_cmds"
> > +#define MPTCP_PM_EV_GRP_NAME "mptcp_events"
>
> We will check if it is OK for Ossama :)
Just to be sure it is clear for everyone, it was decided yesterday at
the weekly meeting to switch to "mptcp_pm_events".
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index e2f40985b450..c2939362f47a 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>
> (...)
>
>> +static int mptcp_event_put_token_and_ssk(struct sk_buff *skb,
>> + const struct mptcp_sock *msk,
>> + const struct sock *ssk)
>> +{
>> + const struct sock *sk = (const struct sock *)msk;
>> + const struct mptcp_subflow_context *sf;
>> + u8 sk_err;
>> +
>> + if (nla_put_u32(skb, MPTCP_ATTR_TOKEN, msk->token))
>> + return -EMSGSIZE;
>> +
>> + if (mptcp_event_add_subflow(skb, ssk))
>> + return -EMSGSIZE;
>> +
>> + sf = mptcp_subflow_ctx(ssk);
>> + if (WARN_ON_ONCE(!sf))
>> + return -EINVAL;
>> +
>> + if (nla_put_u8(skb, MPTCP_ATTR_BACKUP, sf->backup))
>> + return -EMSGSIZE;
>> +
>> + if (ssk->sk_bound_dev_if &&
>> + nla_put_s32(skb, MPTCP_ATTR_IF_IDX, ssk->sk_bound_dev_if))
>> + return -EMSGSIZE;
>> +
>> + sk_err = ssk->sk_err;
>
> Could we have an error set on the msk? (sk->sk_err)
And here, as discussed yesterday, we would keep "ssk->sk_err" and not
use the one from the msk.
I guess Mat and Ossama prefer to quickly validate it before applying
these patches, I can wait before applying them. Or maybe I misunderstood
and I can apply it sooner.
(And if we only need to change the group name, I can fix that when
applying it without having to submit a new version ;-) )
For this patch:
Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 6+ messages in thread* [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support
@ 2021-01-20 17:23 Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2021-01-20 17:23 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 709 bytes --]
On 20/01/2021 18:19, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>> + sk_err = ssk->sk_err;
>>
>> Could we have an error set on the msk? (sk->sk_err)
>
> Userspace interacts with the msk so it should already
> get proper errnos via syscalls, no?
I think likely the module (e.g. mptcpd) interacting with this in-kernel
Netlink PM is not the one which initiated the connection (msk).
On the other hand, we also check that the msk is still in established
mode. What kind of error could we have?
In mptcp.org, we might have taken the one from the meta "just in case".
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 6+ messages in thread
* [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support
@ 2021-01-20 17:15 Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2021-01-20 17:15 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]
Hi Florian,
On 20/01/2021 16:43, Florian Westphal wrote:
> Allow userspace (mptcpd) to subscribe to mptcp genl multicast events.
> This implementation reuses the same event API as the mptcp kernel fork
> to ease integration of existing tools, e.g. mptcpd.
>
> Supported events include:
> 1. start and close of an mptcp connection
> 2. start and close of subflows (joins)
> 3. announce and withdrawals of addresses
> 4. subflow priority (backup/non-backup) change.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
Thank you for the v2!
(...)
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 3674a451a18c..4983ced8e1f8 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -36,6 +36,7 @@ enum {
> /* netlink interface */
> #define MPTCP_PM_NAME "mptcp_pm"
> #define MPTCP_PM_CMD_GRP_NAME "mptcp_pm_cmds"
> +#define MPTCP_PM_EV_GRP_NAME "mptcp_events"
We will check if it is OK for Ossama :)
(...)
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e2f40985b450..c2939362f47a 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
(...)
> +static int mptcp_event_put_token_and_ssk(struct sk_buff *skb,
> + const struct mptcp_sock *msk,
> + const struct sock *ssk)
> +{
> + const struct sock *sk = (const struct sock *)msk;
> + const struct mptcp_subflow_context *sf;
> + u8 sk_err;
> +
> + if (nla_put_u32(skb, MPTCP_ATTR_TOKEN, msk->token))
> + return -EMSGSIZE;
> +
> + if (mptcp_event_add_subflow(skb, ssk))
> + return -EMSGSIZE;
> +
> + sf = mptcp_subflow_ctx(ssk);
> + if (WARN_ON_ONCE(!sf))
> + return -EINVAL;
> +
> + if (nla_put_u8(skb, MPTCP_ATTR_BACKUP, sf->backup))
> + return -EMSGSIZE;
> +
> + if (ssk->sk_bound_dev_if &&
> + nla_put_s32(skb, MPTCP_ATTR_IF_IDX, ssk->sk_bound_dev_if))
> + return -EMSGSIZE;
> +
> + sk_err = ssk->sk_err;
Could we have an error set on the msk? (sk->sk_err)
I don't remember why but in mptcp.org, we take the one of the subflow if
there is one and we fallback to the sk_err of the meta. But I don't
remember in which case we can have sk_err set on the meta. And not sure
if we can have one in the msk :)
It seems that we look for sk_err only in the ssk (subflow.c) and in
mptcp_recvmsg() but maybe just because this code is an adaptation of the
one from TCP?
> + if (sk_err && sk->sk_state == TCP_ESTABLISHED &&
> + nla_put_u8(skb, MPTCP_ATTR_ERROR, sk_err))
> + return -EMSGSIZE;
> +
> + return 0;
> +}
Thank you for the other modifications!
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-26 16:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-20 17:19 [MPTCP] Re: [PATCH v2 mptcp-next 5/5] mptcp: add netlink event support Florian Westphal
-- strict thread matches above, loose matches on Subject: below --
2021-01-26 16:51 Mat Martineau
2021-01-26 15:04 Florian Westphal
2021-01-22 10:55 Matthieu Baerts
2021-01-20 17:23 Matthieu Baerts
2021-01-20 17:15 Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.