All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vladislav Yasevich <vladislav.yasevich@hp.com>
To: Xi Wang <xi.wang@gmail.com>
Cc: netdev@vger.kernel.org, linux-sctp@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Andrei Pelinescu-Onciul <andrei@iptel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose
Date: Tue, 13 Dec 2011 22:15:28 +0000	[thread overview]
Message-ID: <4EE7CE80.9050307@hp.com> (raw)
In-Reply-To: <702540E4-9FD3-4B71-B53A-FE5D4323A898@gmail.com>

On 12/13/2011 05:00 PM, Xi Wang wrote:
> On Dec 12, 2011, at 5:18 PM, Vladislav Yasevich wrote:
>> Hm..  this is a bit strange.  This makes it so that on 32 bit platforms
>> we have one upper bound for autoclose and on 64 we have another even though
>> the type is platform dependent.  This could be considered a regression by
>> applications.
> 
> Either looks good to me.  Timeout limit is essentially different on 32/64
> platforms.

I don't think it really should be different.  Notice that our rto values
remain consistent.  I really thing that this should be consistent from
the user's point of view.

> 
> Another (probably uglier) option is to limit the value on 32-bit platform
> only, like sock_setsockopt() in net/core/sock.c.
> 
> #if (BITS_PER_LONG = 32)
> 	if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ)
> 		sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ;
> #endif

I agree, this is ugly.  It might make more sense to define a max autoclose
value and expose it through /sys.  That way the values remains consistent.

-vlad

> 
>> In addition this would result in confusion to user since the values
>> between setsockopt() and getsockopt() for autoclose would be different.
> 
> Are you suggesting to reject the value and return -EINVAL, rather than
> silently limiting the autoclose value?
> 
> - xi
> 


WARNING: multiple messages have this Message-ID (diff)
From: Vladislav Yasevich <vladislav.yasevich@hp.com>
To: Xi Wang <xi.wang@gmail.com>
Cc: netdev@vger.kernel.org, linux-sctp@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Andrei Pelinescu-Onciul <andrei@iptel.org>,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose
Date: Tue, 13 Dec 2011 17:15:28 -0500	[thread overview]
Message-ID: <4EE7CE80.9050307@hp.com> (raw)
In-Reply-To: <702540E4-9FD3-4B71-B53A-FE5D4323A898@gmail.com>

On 12/13/2011 05:00 PM, Xi Wang wrote:
> On Dec 12, 2011, at 5:18 PM, Vladislav Yasevich wrote:
>> Hm..  this is a bit strange.  This makes it so that on 32 bit platforms
>> we have one upper bound for autoclose and on 64 we have another even though
>> the type is platform dependent.  This could be considered a regression by
>> applications.
> 
> Either looks good to me.  Timeout limit is essentially different on 32/64
> platforms.

I don't think it really should be different.  Notice that our rto values
remain consistent.  I really thing that this should be consistent from
the user's point of view.

> 
> Another (probably uglier) option is to limit the value on 32-bit platform
> only, like sock_setsockopt() in net/core/sock.c.
> 
> #if (BITS_PER_LONG == 32)
> 	if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ)
> 		sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ;
> #endif

I agree, this is ugly.  It might make more sense to define a max autoclose
value and expose it through /sys.  That way the values remains consistent.

-vlad

> 
>> In addition this would result in confusion to user since the values
>> between setsockopt() and getsockopt() for autoclose would be different.
> 
> Are you suggesting to reject the value and return -EINVAL, rather than
> silently limiting the autoclose value?
> 
> - xi
> 

  reply	other threads:[~2011-12-13 22:15 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-09  1:24 [PATCH RESEND] sctp: fix incorrect overflow check on autoclose Xi Wang
2011-12-09  1:24 ` Xi Wang
2011-12-09 17:38 ` Vladislav Yasevich
2011-12-09 17:38   ` Vladislav Yasevich
2011-12-09 18:04   ` Xi Wang
2011-12-09 18:04     ` Xi Wang
2011-12-12 22:18     ` Vladislav Yasevich
2011-12-12 22:18       ` Vladislav Yasevich
2011-12-13 22:00       ` Xi Wang
2011-12-13 22:00         ` Xi Wang
2011-12-13 22:15         ` Vladislav Yasevich [this message]
2011-12-13 22:15           ` Vladislav Yasevich
2011-12-14 21:35           ` Xi Wang
2011-12-14 21:35             ` Xi Wang
2011-12-14 21:48 ` [PATCH v2] " Xi Wang
2011-12-14 21:48   ` Xi Wang
2011-12-15 21:07   ` Vlad Yasevich
2011-12-15 21:07     ` Vlad Yasevich
2011-12-15 22:13     ` Xi Wang
2011-12-15 22:13       ` Xi Wang
2011-12-16 13:00       ` Vlad Yasevich
2011-12-16 13:00         ` Vlad Yasevich
2011-12-16 22:25         ` Xi Wang
2011-12-16 22:25           ` Xi Wang
2011-12-16 22:44 ` [PATCH v3] " Xi Wang
2011-12-16 22:44   ` Xi Wang
2012-01-03 15:52   ` Vladislav Yasevich
2012-01-03 15:52     ` Vladislav Yasevich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4EE7CE80.9050307@hp.com \
    --to=vladislav.yasevich@hp.com \
    --cc=akpm@linux-foundation.org \
    --cc=andrei@iptel.org \
    --cc=davem@davemloft.net \
    --cc=linux-sctp@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=xi.wang@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.