On Wed, Sep 11, 2019 at 8:56 PM Marcelo Ricardo Leitner
[off-list ref] wrote:

On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote:

quoted

On Wed, Sep 11, 2019 at 5:21 PM Xin Long [off-list ref] wrote:

quoted

On Wed, Sep 11, 2019 at 5:03 PM David Laight [off-list ref] wrote:

quoted

From: Xin Long [mailto:lucien.xin@gmail.com]

quoted

Sent: 11 September 2019 09:52
On Tue, Sep 10, 2019 at 9:19 PM David Laight [off-list ref] wrote:

quoted

From: Xin Long

quoted

Sent: 09 September 2019 08:57
Section 7.2 of rfc7829: "Peer Address Thresholds (SCTP_PEER_ADDR_THLDS)
Socket Option" extends 'struct sctp_paddrthlds' with 'spt_pathcpthld'
added to allow a user to change ps_retrans per sock/asoc/transport, as
other 2 paddrthlds: pf_retrans, pathmaxrxt.

Note that ps_retrans is not allowed to be greater than pf_retrans.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h |  1 +
 net/sctp/socket.c         | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index a15cc28..dfd81e1 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h

@@ -1069,6 +1069,7 @@ struct sctp_paddrthlds {
      struct sockaddr_storage spt_address;
      __u16 spt_pathmaxrxt;
      __u16 spt_pathpfthld;
+     __u16 spt_pathcpthld;
 };

 /*

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5e2098b..5b9774d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c

@@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk,

This code does:
        if (optlen < sizeof(struct sctp_paddrthlds))
                return -EINVAL;

here will become:

        if (optlen >= sizeof(struct sctp_paddrthlds)) {
                optlen = sizeof(struct sctp_paddrthlds);
        } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds,
                                            spt_pathcpthld), 4))
                optlen = ALIGN(offsetof(struct sctp_paddrthlds,
                                        spt_pathcpthld), 4);
                val.spt_pathcpthld = 0xffff;
        else {
                return -EINVAL;
        }

Hmmm...
If the kernel has to default 'val.spt_pathcpthld = 0xffff'
then recompiling an existing application with the new uapi
header is going to lead to very unexpected behaviour.

The best you can hope for is that the application memset the
structure to zero.
But more likely it is 'random' on-stack data.

0xffff is a value to disable the feature 'Primary Path Switchover'.
you're right that user might set it to zero unexpectly with their
old application rebuilt.

A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't
matter if users set spt_pathcpthld properly when they're not aware
of this feature, like "sysctl net.sctp.pF_retrans". Looks better?

Sorry for confusing,  "sysctl net.sctp.ps_retrans" is already there
(its value is 0xffff by default),
we just need to do this in sctp_setsockopt_paddr_thresholds():

        if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval,
                           optlen))
                return -EFAULT;

        if (sock_net(sk)->sctp.ps_retrans == 0xffff)
                val.spt_pathcpthld = 0xffff;

I'm confused with the snippets, but if I got them right, this is after
dealing with proper len and could leave val.spt_pathcpthld
uninitialized if the application used the old format and sysctl is !=
0xffff.

right, how about this in sctp_setsockopt_paddr_thresholds():

        offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4);
        if (optlen < offset)
                return -EINVAL;
        if (optlen < sizeof(val) || sock_net(sk)->sctp.ps_retrans == 0xffff) {
                optlen = offset;
                val.spt_pathcpthld = 0xffff;
        } else {
                optlen = sizeof(val);
        }

        if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval,
                           optlen))
                return -EFAULT;

        if (val.spt_pathpfthld > val.spt_pathcpthld)
                return -EINVAL;

Which means we will 'skip' spt_pathcpthld if (it's using old format) or
(ps_retrans is disabled and it's using new format).
Note that  ps_retrans < pf_retrans is not allowed in rfc7829.

and in sctp_getsockopt_paddr_thresholds():

        offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4);
        if (len < offset)
                return -EINVAL;
        if (len < sizeof(val) || sock_net(sk)->sctp.ps_retrans == 0xffff)
                len = offset;
        else
                len = sizeof(val);

        if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len))
                return -EFAULT;

quoted

        if (val.spt_pathpfthld > val.spt_pathcpthld)
                return -EINVAL;

quoted

quoted

        David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)