[patch] ipvs: off by one in set_sctp_state()

[patch] ipvs: off by one in set_sctp_state()

[Dan Carpenter]

The sctp_events[] come from sch->type in set_sctp_state().  They are
between 0-255 so that means we need 256 elements in the array.

I believe that because of how the code is aligned there is normally a
hole after sctp_events[] so this patch doesn't actually change anything.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is static checker stuff.  I'm not very familiar with this code.

diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index 6e14a7b..8646488 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -208,7 +208,7 @@ enum ipvs_sctp_event_t {
 	IP_VS_SCTP_EVE_LAST
 };
 
-static enum ipvs_sctp_event_t sctp_events[255] = {
+static enum ipvs_sctp_event_t sctp_events[256] = {
 	IP_VS_SCTP_EVE_DATA_CLI,
 	IP_VS_SCTP_EVE_INIT_CLI,
 	IP_VS_SCTP_EVE_INIT_ACK_CLI,

Re: [patch] ipvs: off by one in set_sctp_state()

[Julian Anastasov]

	Hello,

On Sat, 20 Apr 2013, Dan Carpenter wrote:

> The sctp_events[] come from sch->type in set_sctp_state().  They are
> between 0-255 so that means we need 256 elements in the array.
> 
> I believe that because of how the code is aligned there is normally a
> hole after sctp_events[] so this patch doesn't actually change anything.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is static checker stuff.  I'm not very familiar with this code.
> 
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index 6e14a7b..8646488 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -208,7 +208,7 @@ enum ipvs_sctp_event_t {
>  	IP_VS_SCTP_EVE_LAST
>  };
>  
> -static enum ipvs_sctp_event_t sctp_events[255] = {
> +static enum ipvs_sctp_event_t sctp_events[256] = {
>  	IP_VS_SCTP_EVE_DATA_CLI,
>  	IP_VS_SCTP_EVE_INIT_CLI,
>  	IP_VS_SCTP_EVE_INIT_ACK_CLI,

	There are more confusing (still, non-fatal)
problems in this IPVS-SCTP support, eg.

        if (direction == IP_VS_DIR_OUTPUT)
-               event++;
+               event *= 2;

	I guess we are running with wrong timeouts.

	Also, I'm not sure we support properly the
one-way states as done for TCP (IP_VS_DIR_INPUT_ONLY).
May be this code deserves more serious review, for example,
net/netfilter/nf_conntrack_proto_sctp.c looks as good
source for comparison.

	Anyways, this change looks correct to me,

Acked-by: Julian Anastasov <ja@ssi.bg>

Re: [patch] ipvs: off by one in set_sctp_state()

[Simon Horman]

On Sat, Apr 20, 2013 at 03:25:21PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sat, 20 Apr 2013, Dan Carpenter wrote:
> 
> > The sctp_events[] come from sch->type in set_sctp_state().  They are
> > between 0-255 so that means we need 256 elements in the array.
> > 
> > I believe that because of how the code is aligned there is normally a
> > hole after sctp_events[] so this patch doesn't actually change anything.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > This is static checker stuff.  I'm not very familiar with this code.
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > index 6e14a7b..8646488 100644
> > --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > @@ -208,7 +208,7 @@ enum ipvs_sctp_event_t {
> >  	IP_VS_SCTP_EVE_LAST
> >  };
> >  
> > -static enum ipvs_sctp_event_t sctp_events[255] = {
> > +static enum ipvs_sctp_event_t sctp_events[256] = {
> >  	IP_VS_SCTP_EVE_DATA_CLI,
> >  	IP_VS_SCTP_EVE_INIT_CLI,
> >  	IP_VS_SCTP_EVE_INIT_ACK_CLI,
> 
> 	There are more confusing (still, non-fatal)
> problems in this IPVS-SCTP support, eg.
> 
>         if (direction == IP_VS_DIR_OUTPUT)
> -               event++;
> +               event *= 2;
> 
> 	I guess we are running with wrong timeouts.

IMHO there seem to be many problems with SCTP, but it is good to
fix the ones we find as we find them.

Would you like to make a patch for the above change or should I?

> 	Also, I'm not sure we support properly the
> one-way states as done for TCP (IP_VS_DIR_INPUT_ONLY).
> May be this code deserves more serious review, for example,
> net/netfilter/nf_conntrack_proto_sctp.c looks as good
> source for comparison.

I believe it does need a more serious review.

> 	Anyways, this change looks correct to me,
> 
> Acked-by: Julian Anastasov <ja@ssi.bg>

I have queued-up this change in ipvs-next.
Thanks Dan & Julian.

Re: [patch] ipvs: off by one in set_sctp_state()

[Julian Anastasov]

	Hello,

On Mon, 22 Apr 2013, Simon Horman wrote:

> > 	There are more confusing (still, non-fatal)
> > problems in this IPVS-SCTP support, eg.
> > 
> >         if (direction == IP_VS_DIR_OUTPUT)
> > -               event++;
> > +               event *= 2;
> > 
> > 	I guess we are running with wrong timeouts.
> 
> IMHO there seem to be many problems with SCTP, but it is good to
> fix the ones we find as we find them.

	At the time I found it (during IPVS optimizations
development), it didn't looked fatal, I preferred to
allocate more time for SCTP for debugging.

> Would you like to make a patch for the above change or should I?

	May be the code is correct, my mistake. I was
confused from the order in sctp_events[] but ipvs_sctp_event_t
allocates values for _SER states.

> > 	Also, I'm not sure we support properly the
> > one-way states as done for TCP (IP_VS_DIR_INPUT_ONLY).
> > May be this code deserves more serious review, for example,
> > net/netfilter/nf_conntrack_proto_sctp.c looks as good
> > source for comparison.
> 
> I believe it does need a more serious review.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [patch] ipvs: off by one in set_sctp_state()

[Moahn Reddy]

Hi,

I see there is no problem in the code regarding the state change. And 
the thing why I took 255 in the sctp_events array is that as per the 
sctp specification, the 255 message is reserved, so I thought 0 to 254 
messages are enough.

Do you see any problem with the ipvs sctp code in the field?. Please let 
me know if you see any such, I will try to fix.

Thanks,
Mohan

On Monday 22 April 2013 11:33 AM, Julian Anastasov wrote:
> 	Hello,
>
> On Mon, 22 Apr 2013, Simon Horman wrote:
>
>>> 	There are more confusing (still, non-fatal)
>>> problems in this IPVS-SCTP support, eg.
>>>
>>>          if (direction == IP_VS_DIR_OUTPUT)
>>> -               event++;
>>> +               event *= 2;
>>>
>>> 	I guess we are running with wrong timeouts.
>> IMHO there seem to be many problems with SCTP, but it is good to
>> fix the ones we find as we find them.
> 	At the time I found it (during IPVS optimizations
> development), it didn't looked fatal, I preferred to
> allocate more time for SCTP for debugging.
>
>> Would you like to make a patch for the above change or should I?
> 	May be the code is correct, my mistake. I was
> confused from the order in sctp_events[] but ipvs_sctp_event_t
> allocates values for _SER states.
>
>>> 	Also, I'm not sure we support properly the
>>> one-way states as done for TCP (IP_VS_DIR_INPUT_ONLY).
>>> May be this code deserves more serious review, for example,
>>> net/netfilter/nf_conntrack_proto_sctp.c looks as good
>>> source for comparison.
>> I believe it does need a more serious review.
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] ipvs: off by one in set_sctp_state()

[Dan Carpenter]

On Mon, Apr 22, 2013 at 02:47:10PM +0530, Moahn Reddy wrote:
> Hi,
> 
> I see there is no problem in the code regarding the state change.
> And the thing why I took 255 in the sctp_events array is that as per
> the sctp specification, the 255 message is reserved, so I thought 0
> to 254 messages are enough.

The issue is can chunk_type in set_sctp_state() be 255?  How is
this prevented in the code?

regards,
dan carpenter

Re: [patch] ipvs: off by one in set_sctp_state()

[Moahn Reddy]

Hi Dan,

I am not saying that that is not a problem. Yes, we need to fix that 
array issue. I just said that I thought 0 to 254 enough at that time.

Thanks,
Mohan

On Monday 22 April 2013 03:00 PM, Dan Carpenter wrote:
> On Mon, Apr 22, 2013 at 02:47:10PM +0530, Moahn Reddy wrote:
>> Hi,
>>
>> I see there is no problem in the code regarding the state change.
>> And the thing why I took 255 in the sctp_events array is that as per
>> the sctp specification, the 255 message is reserved, so I thought 0
>> to 254 messages are enough.
> The issue is can chunk_type in set_sctp_state() be 255?  How is
> this prevented in the code?
>
> regards,
> dan carpenter
>

Re: [patch] ipvs: off by one in set_sctp_state()

[Simon Horman]

On Mon, Apr 22, 2013 at 09:03:28AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 22 Apr 2013, Simon Horman wrote:
> 
> > > 	There are more confusing (still, non-fatal)
> > > problems in this IPVS-SCTP support, eg.
> > > 
> > >         if (direction == IP_VS_DIR_OUTPUT)
> > > -               event++;
> > > +               event *= 2;
> > > 
> > > 	I guess we are running with wrong timeouts.
> > 
> > IMHO there seem to be many problems with SCTP, but it is good to
> > fix the ones we find as we find them.
> 
> 	At the time I found it (during IPVS optimizations
> development), it didn't looked fatal, I preferred to
> allocate more time for SCTP for debugging.
> 
> > Would you like to make a patch for the above change or should I?
> 
> 	May be the code is correct, my mistake. I was
> confused from the order in sctp_events[] but ipvs_sctp_event_t
> allocates values for _SER states.

Thanks, it sounds like we should study things more carefully
before making any changes.

> > > 	Also, I'm not sure we support properly the
> > > one-way states as done for TCP (IP_VS_DIR_INPUT_ONLY).
> > > May be this code deserves more serious review, for example,
> > > net/netfilter/nf_conntrack_proto_sctp.c looks as good
> > > source for comparison.
> > 
> > I believe it does need a more serious review.
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
>