[PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[David Miller]

This patch series eliminates the explicit reference to the skb list
implementation via skb->prev dereferences.

The approach used is to pass a non-empty skb list around instead of an
event skb object which may or may not be on a list.

I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
reviewing previous versions of this series.

Testing would be very much appreciated, in addition to the review of
course.

v3 --> v4: Fix the logic in patch #4 so that we don't miss cases
       	   where we should add event to the on-stack temp list.

Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[Neil Horman]

On Mon, Mar 11, 2019 at 08:09:19PM -0700, David Miller wrote:
> 
> This patch series eliminates the explicit reference to the skb list
> implementation via skb->prev dereferences.
> 
> The approach used is to pass a non-empty skb list around instead of an
> event skb object which may or may not be on a list.
> 
> I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
> reviewing previous versions of this series.
> 
> Testing would be very much appreciated, in addition to the review of
> course.
> 
> v3 --> v4: Fix the logic in patch #4 so that we don't miss cases
>        	   where we should add event to the on-stack temp list.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 

I think this looks good now, thanks for taking the time on it Dave

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[Marcelo Ricardo Leitner]

On Mon, Mar 11, 2019 at 08:09:19PM -0700, David Miller wrote:
> 
> This patch series eliminates the explicit reference to the skb list
> implementation via skb->prev dereferences.
> 
> The approach used is to pass a non-empty skb list around instead of an
> event skb object which may or may not be on a list.
> 
> I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
> reviewing previous versions of this series.
> 
> Testing would be very much appreciated, in addition to the review of
> course.
> 
> v3 --> v4: Fix the logic in patch #4 so that we don't miss cases
>        	   where we should add event to the on-stack temp list.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

Code review LGTM too.
Running some tests, should be done by the end of the day.

  Marcelo

Re: [PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[Marcelo Ricardo Leitner]

On Wed, Mar 13, 2019 at 01:06:21PM -0300, Marcelo Ricardo Leitner wrote:
> On Mon, Mar 11, 2019 at 08:09:19PM -0700, David Miller wrote:
> > 
> > This patch series eliminates the explicit reference to the skb list
> > implementation via skb->prev dereferences.
> > 
> > The approach used is to pass a non-empty skb list around instead of an
> > event skb object which may or may not be on a list.
> > 
> > I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
> > reviewing previous versions of this series.
> > 
> > Testing would be very much appreciated, in addition to the review of
> > course.
> > 
> > v3 --> v4: Fix the logic in patch #4 so that we don't miss cases
> >        	   where we should add event to the on-stack temp list.
> > 
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Code review LGTM too.
> Running some tests, should be done by the end of the day.

Tests were good with the exception of one unrelated issue. ACK from my
side thus.


Xin, would you mind carrying forward this fix? I think we need to fix
the other sockopts too. Thanks!

---8<---

From ddba48476b343dce84a82036e7914a1f7ac3f0c8 Mon Sep 17 00:00:00 2001
Message-Id: <ddba48476b343dce84a82036e7914a1f7ac3f0c8.1552526507.git.marcelo.leitner@gmail.com>
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Wed, 13 Mar 2019 22:13:52 -0300
Subject: [PATCH] net/sctp: fix ignoring asoc_id for tcp-style sockets on
 setsockopt

Currently if the user pass an invalid asoc_id to SCTP_DEFAULT_SEND_PARAM
on a TCP-style socket, it will silently ignore the new parameters.
That's because after not finding an asoc, it is checking asoc_id against
the known values of CURRENT/FUTURE/ALL values and that fails to match.

IOW, if the user supplies an invalid asoc id or not, it should either
match the current asoc or the socket itself so that it will inherit
these later. Fixes it by forcing asoc_id to SCTP_FUTURE_ASSOC in case it
is a TCP-style socket without an asoc, so that the values get set on the
socket.

Fixes: 707e45b3dc5a ("sctp: use SCTP_FUTURE_ASSOC and add SCTP_CURRENT_ASSOC for SCTP_DEFAULT_SEND_PARAM sockopt")
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/socket.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 533207dbeae955604a6304a8f67b65c9f580fd05..85205a2d9a7dc5c289a94bf4672b240040cbb546 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3024,6 +3024,9 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
 		return 0;
 	}
 
+	if (sctp_style(sk, TCP))
+		info.sinfo_assoc_id = SCTP_FUTURE_ASSOC;
+
 	if (info.sinfo_assoc_id == SCTP_FUTURE_ASSOC ||
 	    info.sinfo_assoc_id == SCTP_ALL_ASSOC) {
 		sp->default_stream = info.sinfo_stream;
-- 
2.20.1

Re: [PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[Xin Long]

On Thu, Mar 14, 2019 at 9:23 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Wed, Mar 13, 2019 at 01:06:21PM -0300, Marcelo Ricardo Leitner wrote:
> > On Mon, Mar 11, 2019 at 08:09:19PM -0700, David Miller wrote:
> > >
> > > This patch series eliminates the explicit reference to the skb list
> > > implementation via skb->prev dereferences.
> > >
> > > The approach used is to pass a non-empty skb list around instead of an
> > > event skb object which may or may not be on a list.
> > >
> > > I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
> > > reviewing previous versions of this series.
> > >
> > > Testing would be very much appreciated, in addition to the review of
> > > course.
> > >
> > > v3 --> v4: Fix the logic in patch #4 so that we don't miss cases
> > >                where we should add event to the on-stack temp list.
> > >
> > > Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> > Code review LGTM too.
> > Running some tests, should be done by the end of the day.
>
> Tests were good with the exception of one unrelated issue. ACK from my
> side thus.
>
>
> Xin, would you mind carrying forward this fix? I think we need to fix
> the other sockopts too. Thanks!
Sure, it seems to affect quite a few socket opts.
Somehow I made sense of it before.
So with this patch, asoc_id will be completed ignored by TCP type socket?

>
> ---8<---
>
> From ddba48476b343dce84a82036e7914a1f7ac3f0c8 Mon Sep 17 00:00:00 2001
> Message-Id: <ddba48476b343dce84a82036e7914a1f7ac3f0c8.1552526507.git.marcelo.leitner@gmail.com>
> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Date: Wed, 13 Mar 2019 22:13:52 -0300
> Subject: [PATCH] net/sctp: fix ignoring asoc_id for tcp-style sockets on
>  setsockopt
>
> Currently if the user pass an invalid asoc_id to SCTP_DEFAULT_SEND_PARAM
> on a TCP-style socket, it will silently ignore the new parameters.
> That's because after not finding an asoc, it is checking asoc_id against
> the known values of CURRENT/FUTURE/ALL values and that fails to match.
>
> IOW, if the user supplies an invalid asoc id or not, it should either
> match the current asoc or the socket itself so that it will inherit
> these later. Fixes it by forcing asoc_id to SCTP_FUTURE_ASSOC in case it
> is a TCP-style socket without an asoc, so that the values get set on the
> socket.
>
> Fixes: 707e45b3dc5a ("sctp: use SCTP_FUTURE_ASSOC and add SCTP_CURRENT_ASSOC for SCTP_DEFAULT_SEND_PARAM sockopt")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>  net/sctp/socket.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 533207dbeae955604a6304a8f67b65c9f580fd05..85205a2d9a7dc5c289a94bf4672b240040cbb546 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3024,6 +3024,9 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
>                 return 0;
>         }
>
> +       if (sctp_style(sk, TCP))
> +               info.sinfo_assoc_id = SCTP_FUTURE_ASSOC;
> +
>         if (info.sinfo_assoc_id == SCTP_FUTURE_ASSOC ||
>             info.sinfo_assoc_id == SCTP_ALL_ASSOC) {
>                 sp->default_stream = info.sinfo_stream;
> --
> 2.20.1
>

Re: [PATCH RFC v4 0/5] SCTP: Event skb list overhaul.

[Marcelo Ricardo Leitner]

On Thu, Mar 14, 2019 at 03:51:59PM +0800, Xin Long wrote:
> On Thu, Mar 14, 2019 at 9:23 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Wed, Mar 13, 2019 at 01:06:21PM -0300, Marcelo Ricardo Leitner wrote:
> > > On Mon, Mar 11, 2019 at 08:09:19PM -0700, David Miller wrote:
> > > >
> > > > This patch series eliminates the explicit reference to the skb list
> > > > implementation via skb->prev dereferences.
> > > >
> > > > The approach used is to pass a non-empty skb list around instead of an
> > > > event skb object which may or may not be on a list.
> > > >
> > > > I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
> > > > reviewing previous versions of this series.
> > > >
> > > > Testing would be very much appreciated, in addition to the review of
> > > > course.
> > > >
> > > > v3 --> v4: Fix the logic in patch #4 so that we don't miss cases
> > > >                where we should add event to the on-stack temp list.
> > > >
> > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > >
> > > Code review LGTM too.
> > > Running some tests, should be done by the end of the day.
> >
> > Tests were good with the exception of one unrelated issue. ACK from my
> > side thus.
> >
> >
> > Xin, would you mind carrying forward this fix? I think we need to fix
> > the other sockopts too. Thanks!
> Sure, it seems to affect quite a few socket opts.

Thanks.

> Somehow I made sense of it before.

For me too :-)

> So with this patch, asoc_id will be completed ignored by TCP type socket?

Yes. It already is when looking for the asoc, by sctp_id2assoc(). Just
need to re-ignore it if there is no asoc yet.

> 
> >
> > ---8<---
> >
> > From ddba48476b343dce84a82036e7914a1f7ac3f0c8 Mon Sep 17 00:00:00 2001
> > Message-Id: <ddba48476b343dce84a82036e7914a1f7ac3f0c8.1552526507.git.marcelo.leitner@gmail.com>
> > From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > Date: Wed, 13 Mar 2019 22:13:52 -0300
> > Subject: [PATCH] net/sctp: fix ignoring asoc_id for tcp-style sockets on
> >  setsockopt
> >
> > Currently if the user pass an invalid asoc_id to SCTP_DEFAULT_SEND_PARAM
> > on a TCP-style socket, it will silently ignore the new parameters.
> > That's because after not finding an asoc, it is checking asoc_id against
> > the known values of CURRENT/FUTURE/ALL values and that fails to match.
> >
> > IOW, if the user supplies an invalid asoc id or not, it should either
> > match the current asoc or the socket itself so that it will inherit
> > these later. Fixes it by forcing asoc_id to SCTP_FUTURE_ASSOC in case it
> > is a TCP-style socket without an asoc, so that the values get set on the
> > socket.
> >
> > Fixes: 707e45b3dc5a ("sctp: use SCTP_FUTURE_ASSOC and add SCTP_CURRENT_ASSOC for SCTP_DEFAULT_SEND_PARAM sockopt")
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > ---
> >  net/sctp/socket.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 533207dbeae955604a6304a8f67b65c9f580fd05..85205a2d9a7dc5c289a94bf4672b240040cbb546 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -3024,6 +3024,9 @@ static int sctp_setsockopt_default_send_param(struct sock *sk,
> >                 return 0;
> >         }
> >
> > +       if (sctp_style(sk, TCP))
> > +               info.sinfo_assoc_id = SCTP_FUTURE_ASSOC;
> > +
> >         if (info.sinfo_assoc_id == SCTP_FUTURE_ASSOC ||
> >             info.sinfo_assoc_id == SCTP_ALL_ASSOC) {
> >                 sp->default_stream = info.sinfo_stream;
> > --
> > 2.20.1
> >