Hi,

2018-06-28 15:40 GMT+09:00 Xin Long [off-list ref]:

On Tue, Jun 26, 2018 at 8:02 PM, 吉藤英明
[off-list ref] wrote:

quoted

2018-06-26 13:33 GMT+09:00 Xin Long [off-list ref]:

quoted

On Tue, Jun 26, 2018 at 12:31 AM, Marcelo Ricardo Leitner
[off-list ref] wrote:

quoted

Hi,

On Tue, Jun 26, 2018 at 01:12:00AM +0900, 吉藤英明 wrote:

quoted

Hi,

2018-06-25 22:03 GMT+09:00 Marcelo Ricardo Leitner [off-list ref]:

quoted

On Mon, Jun 25, 2018 at 07:28:47AM -0400, Neil Horman wrote:

quoted

On Mon, Jun 25, 2018 at 04:31:26PM +0900, David Miller wrote:

quoted

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 25 Jun 2018 10:14:35 +0800

quoted

 struct sctp_paddrparams {

@@ -773,6 +775,8 @@ struct sctp_paddrparams {
  __u32                   spp_pathmtu;
  __u32                   spp_sackdelay;
  __u32                   spp_flags;
+ __u32                   spp_ipv6_flowlabel;
+ __u8                    spp_dscp;
 } __attribute__((packed, aligned(4)));

I don't think you can change the size of this structure like this.

This check in sctp_setsockopt_peer_addr_params():

    if (optlen != sizeof(struct sctp_paddrparams))
            return -EINVAL;

is going to trigger in old kernels when executing programs
built against the new struct definition.

That will happen, yes, but do we really care about being future-proof
here? I mean: if we also update such check(s) to support dealing with
smaller-than-supported structs, newer kernels will be able to run
programs built against the old struct, and the new one; while building
using newer headers and running on older kernel may fool the
application in other ways too (like enabling support for something
that is available on newer kernel and that is not present in the older
one).

We should not break existing apps.
We still accept apps of pre-2.4 era without sin6_scope_id
(e.g., net/ipv6/af_inet6.c:inet6_bind()).

Yes. That's what I tried to say. That is supporting an old app built
with old kernel headers and running on a newer kernel, and not the
other way around (an app built with fresh headers and running on an
old kernel).

To make it, I will update the check like:

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 1df5d07..c949d8c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c

@@ -2715,13 +2715,18 @@ static int

sctp_setsockopt_peer_addr_params(struct sock *sk,
        struct sctp_sock        *sp = sctp_sk(sk);
        int error;
        int hb_change, pmtud_change, sackdelay_change;
+       int plen = sizeof(params);
+       int old_plen = plen - sizeof(u32) * 2;

if (optlen < offsetof(struct sctp_paddrparams, spp_ipv6_flowlabel))
maybe?

Hi, yoshfuji,
offsetof() is better. thank you.

quoted

quoted

-       if (optlen != sizeof(struct sctp_paddrparams))
+       if (optlen != plen && optlen != old_plen)
                return -EINVAL;

        if (copy_from_user(&params, optval, optlen))
                return -EFAULT;

+       if (optlen == old_plen)
+               params.spp_flags &= ~(SPP_DSCP | SPP_IPV6_FLOWLABEL);

I think we should return -EINVAL if size is not new one.

Sorry, if we returned  -EINVAL when size is the old one,
how can we guarantee an old app built with old kernel
headers and running on a newer kernel works well?
or you meant?
if ((params.spp_flags & (SPP_DSCP | SPP_IPV6_FLOWLABEL)) &&
    optlen != plen)
        return EINVAL;

Yes, I meant this (it should be -EINVAL though).

quoted

--yoshfuji

quoted

+
        /* Validate flags and value parameters. */
        hb_change        = params.spp_flags & SPP_HB;
        pmtud_change     = params.spp_flags & SPP_PMTUD;

@@ -5591,10 +5596,13 @@ static int

sctp_getsockopt_peer_addr_params(struct sock *sk, int len,
        struct sctp_transport   *trans = NULL;
        struct sctp_association *asoc = NULL;
        struct sctp_sock        *sp = sctp_sk(sk);
+       int plen = sizeof(params);
+       int old_plen = plen - sizeof(u32) * 2;

-       if (len < sizeof(struct sctp_paddrparams))
+       if (len < old_plen)
                return -EINVAL;
-       len = sizeof(struct sctp_paddrparams);
+
+       len = len >= plen ? plen : old_plen;
        if (copy_from_user(&params, optval, len))
                return -EFAULT;

does it look ok to you?