From: Marcelo L. <mle...@re...> - 2011-10-04 17:48:48
Attachments:
setkey-do_not_shrink_socket_buffers.patch
|
Hi all, I've noticed setkey tool can't use racoon.cfg pfkey_buffer, as it simply doesn't use that file. But it tries to raise the socket snd/rcv buffers trying to handle more policies. But it ends up shrinking the buffers if you have sysctl [rw]mem_default values tuned. We have a report of setkey tool failing due to small socket buffers for dumping a big policy database. So I suggest using the attached patch. setkey tool will not decrease the buffers anymore. What do you think? Any other ideas? I tried to be low impact on this change, as probably we will backport it to ipsec-tools-0.6.5. Thanks, Marcelo Leitner. |
From: Marcelo L. <mle...@re...> - 2011-10-04 18:14:58
Attachments:
setkey-do_not_shrink_socket_buffers_v2.patch
|
Sorry, I emailed a previous version of the patch, that doesn't even compile. Please, refer to this new one. Thanks, Marcelo On 10/04/2011 02:48 PM, Marcelo Leitner wrote: > Hi all, > > I've noticed setkey tool can't use racoon.cfg pfkey_buffer, as it > simply doesn't use that file. But it tries to raise the socket snd/rcv > buffers trying to handle more policies. > > But it ends up shrinking the buffers if you have sysctl > [rw]mem_default values tuned. > > We have a report of setkey tool failing due to small socket buffers > for dumping a big policy database. > > So I suggest using the attached patch. setkey tool will not decrease > the buffers anymore. > > What do you think? Any other ideas? I tried to be low impact on this > change, as probably we will backport it to ipsec-tools-0.6.5. > > Thanks, > Marcelo Leitner. > > > ------------------------------------------------------------------------------ > All the data continuously generated in your IT infrastructure contains a > definitive record of customers, application performance, security > threats, fraudulent activity and more. Splunk takes this data and makes > sense of it. Business sense. IT sense. Common sense. > http://p.sf.net/sfu/splunk-d2dcopy1 > > > _______________________________________________ > Ipsec-tools-devel mailing list > Ips...@li... > https://lists.sourceforge.net/lists/listinfo/ipsec-tools-devel |
From: Marcelo L. <mle...@re...> - 2011-11-10 13:04:42
Attachments:
setkey-do_not_shrink_socket_buffers_v2.patch
|
Dear ipsec-tools developers, Do we have any news on this patch? I couldn't find any by myself. I'm re-attaching it for your convenience. Thanks, Marcelo. On 10/04/2011 03:14 PM, Marcelo Leitner wrote: > Sorry, I emailed a previous version of the patch, that doesn't even > compile. > > Please, refer to this new one. > > Thanks, > Marcelo > > On 10/04/2011 02:48 PM, Marcelo Leitner wrote: >> Hi all, >> >> I've noticed setkey tool can't use racoon.cfg pfkey_buffer, as it >> simply doesn't use that file. But it tries to raise the socket >> snd/rcv buffers trying to handle more policies. >> >> But it ends up shrinking the buffers if you have sysctl >> [rw]mem_default values tuned. >> >> We have a report of setkey tool failing due to small socket buffers >> for dumping a big policy database. >> >> So I suggest using the attached patch. setkey tool will not decrease >> the buffers anymore. >> >> What do you think? Any other ideas? I tried to be low impact on this >> change, as probably we will backport it to ipsec-tools-0.6.5. >> >> Thanks, >> Marcelo Leitner. >> >> >> ------------------------------------------------------------------------------ >> >> All the data continuously generated in your IT infrastructure contains a >> definitive record of customers, application performance, security >> threats, fraudulent activity and more. Splunk takes this data and makes >> sense of it. Business sense. IT sense. Common sense. >> http://p.sf.net/sfu/splunk-d2dcopy1 >> >> >> _______________________________________________ >> Ipsec-tools-devel mailing list >> Ips...@li... >> https://lists.sourceforge.net/lists/listinfo/ipsec-tools-devel > > > ------------------------------------------------------------------------------ > All the data continuously generated in your IT infrastructure contains a > definitive record of customers, application performance, security > threats, fraudulent activity and more. Splunk takes this data and makes > sense of it. Business sense. IT sense. Common sense. > http://p.sf.net/sfu/splunk-d2dcopy1 > > > _______________________________________________ > Ipsec-tools-devel mailing list > Ips...@li... > https://lists.sourceforge.net/lists/listinfo/ipsec-tools-devel |
From: Timo T. <tim...@ik...> - 2011-11-12 13:33:18
|
Hi, On 11/10/2011 03:04 PM, Marcelo Leitner wrote: > Do we have any news on this patch? I couldn't find any by myself. > > I'm re-attaching it for your convenience. Sorry for the delay. I (and probably the others) have been busy lately. > diff -up ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig ipsec-tools-0.8.0/src/libipsec/pfkey.c > --- ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig 2011-10-04 11:48:43.534727761 -0300 > +++ ipsec-tools-0.8.0/src/libipsec/pfkey.c 2011-10-04 15:13:02.520726582 -0300 > @@ -1783,7 +1783,10 @@ int > pfkey_open(void) > { > int so; > - int bufsiz = 128 * 1024; /*is 128K enough?*/ > + int bufsiz_current, bufsiz_wanted; > + int ret; > + socklen_t len; > +#define MAX_SO_RCVBUF (16*1024*1024) > > if ((so = socket(PF_KEY, SOCK_RAW, PF_KEY_V2)) < 0) { > __ipsec_set_strerror(strerror(errno)); > @@ -1794,15 +1797,29 @@ pfkey_open(void) > * This is a temporary workaround for KAME PR 154. > * Don't really care even if it fails. > */ > - (void)setsockopt(so, SOL_SOCKET, SO_SNDBUF, &bufsiz, sizeof(bufsiz)); > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > - bufsiz = 256 * 1024; > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > - bufsiz = 512 * 1024; > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > - bufsiz = 1024 * 1024; > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > + // Try to have 128k. If we have more, do not lower it. Use ANSI C comments, not C++. > + bufsiz_wanted = 128 * 1024; > + ret = getsockopt(so, SOL_SOCKET, SO_SNDBUF, > + &bufsiz_current, &len); > + if ((ret < 0) || (bufsiz_current < bufsiz_wanted)) > + setsockopt(so, SOL_SOCKET, SO_SNDBUF, > + &bufsiz_wanted, sizeof(bufsiz_wanted)); Better keep the (void) casts here to keep some nitpicking compilers happy. > + > + // Try to have have at least 2MB. If we have more, do not lower it. > + bufsiz_wanted = 2 * 1024 * 1024; > + ret = getsockopt(so, SOL_SOCKET, SO_RCVBUF, > + &bufsiz_current, &len); > + > + if ((ret < 0) || (bufsiz_current < bufsiz_wanted)) { > + bufsiz_wanted = 128 * 1024; > + for (; bufsiz_wanted < MAX_SO_RCVBUF; bufsiz_wanted *= 2) { > + if (setsockopt(so, SOL_SOCKET, SO_RCVBUF, > + &bufsiz_wanted, sizeof(bufsiz_wanted)) < 0) > + break; > + } > + } What if we had 240k buffer. We now go and set the size to 128k, and if setting 256k fails, we end up making it smaller after all. Could we instead have for loop going from 128k -> 2MB and only trying to set it if the value is larger then what was the previous buffer. Actually, I changed my mind. Let's have a for loop from 2MB -> 128k that breaks when the buffer size to set is smaller then previously set size, or when we first time successfully set the size. > __ipsec_errcode = EIPSEC_NO_ERROR; > +#undef MAX_SO_RCVBUF > return so; > } Can we also ditch MAX_SO_RCVBUF? It's misleading name (it's still the bufsize_wanted and nothing else) and it's used only once anyway. Otherwise it looks good to go. Please resend with fixes and I'll push to HEAD and 0.8-branch. Thanks, Timo |
From: Marcelo L. <mle...@re...> - 2011-11-14 13:02:00
Attachments:
setkey-do_not_shrink_socket_buffers_v3.patch
|
Hi there Timo, On 11/12/2011 11:33 AM, Timo Teräs wrote: > Hi, > > On 11/10/2011 03:04 PM, Marcelo Leitner wrote: >> Do we have any news on this patch? I couldn't find any by myself. >> >> I'm re-attaching it for your convenience. > Sorry for the delay. I (and probably the others) have been busy lately. No problem at all! Just wasn't sure if it had went through right. >> diff -up ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig ipsec-tools-0.8.0/src/libipsec/pfkey.c >> --- ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig 2011-10-04 11:48:43.534727761 -0300 >> +++ ipsec-tools-0.8.0/src/libipsec/pfkey.c 2011-10-04 15:13:02.520726582 -0300 >> @@ -1783,7 +1783,10 @@ int >> pfkey_open(void) >> { >> int so; >> - int bufsiz = 128 * 1024; /*is 128K enough?*/ >> + int bufsiz_current, bufsiz_wanted; >> + int ret; >> + socklen_t len; >> +#define MAX_SO_RCVBUF (16*1024*1024) >> >> if ((so = socket(PF_KEY, SOCK_RAW, PF_KEY_V2))< 0) { >> __ipsec_set_strerror(strerror(errno)); >> @@ -1794,15 +1797,29 @@ pfkey_open(void) >> * This is a temporary workaround for KAME PR 154. >> * Don't really care even if it fails. >> */ >> - (void)setsockopt(so, SOL_SOCKET, SO_SNDBUF,&bufsiz, sizeof(bufsiz)); >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> - bufsiz = 256 * 1024; >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> - bufsiz = 512 * 1024; >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> - bufsiz = 1024 * 1024; >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> + // Try to have 128k. If we have more, do not lower it. > Use ANSI C comments, not C++. Ok >> + bufsiz_wanted = 128 * 1024; >> + ret = getsockopt(so, SOL_SOCKET, SO_SNDBUF, >> + &bufsiz_current,&len); >> + if ((ret< 0) || (bufsiz_current< bufsiz_wanted)) >> + setsockopt(so, SOL_SOCKET, SO_SNDBUF, >> + &bufsiz_wanted, sizeof(bufsiz_wanted)); > Better keep the (void) casts here to keep some nitpicking compilers happy. > Ok too >> + >> + // Try to have have at least 2MB. If we have more, do not lower it. >> + bufsiz_wanted = 2 * 1024 * 1024; >> + ret = getsockopt(so, SOL_SOCKET, SO_RCVBUF, >> + &bufsiz_current,&len); >> + >> + if ((ret< 0) || (bufsiz_current< bufsiz_wanted)) { >> + bufsiz_wanted = 128 * 1024; >> + for (; bufsiz_wanted< MAX_SO_RCVBUF; bufsiz_wanted *= 2) { >> + if (setsockopt(so, SOL_SOCKET, SO_RCVBUF, >> + &bufsiz_wanted, sizeof(bufsiz_wanted))< 0) >> + break; >> + } >> + } > What if we had 240k buffer. We now go and set the size to 128k, and if > setting 256k fails, we end up making it smaller after all. > > Could we instead have for loop going from 128k -> 2MB and only trying to > set it if the value is larger then what was the previous buffer. > > Actually, I changed my mind. Let's have a for loop from 2MB -> 128k that > breaks when the buffer size to set is smaller then previously set size, > or when we first time successfully set the size. Interesting approach. Seems much cleaner indeed. >> __ipsec_errcode = EIPSEC_NO_ERROR; >> +#undef MAX_SO_RCVBUF >> return so; >> } > Can we also ditch MAX_SO_RCVBUF? It's misleading name (it's still the > bufsize_wanted and nothing else) and it's used only once anyway. Ok > Otherwise it looks good to go. Please resend with fixes and I'll push to > HEAD and 0.8-branch. Here goes the 3rd version. Please see what you think. We can update it again if needed, no problem. Thanks for all your patience and time, Timo. Best regards, Marcelo. |
From: Timo T. <tim...@ik...> - 2011-11-14 13:14:43
|
On 11/14/2011 03:01 PM, Marcelo Leitner wrote: > Here goes the 3rd version. Please see what you think. We can update it > again if needed, no problem. > > Thanks for all your patience and time, Timo. > diff -up ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig ipsec-tools-0.8.0/src/libipsec/pfkey.c > --- ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig 2011-10-04 11:48:43.534727761 -0300 > +++ ipsec-tools-0.8.0/src/libipsec/pfkey.c 2011-11-14 10:40:42.555775797 -0200 > @@ -1783,7 +1783,9 @@ int > pfkey_open(void) > { > int so; > - int bufsiz = 128 * 1024; /*is 128K enough?*/ > + int bufsiz_current, bufsiz_wanted; > + int ret; > + socklen_t len; > > if ((so = socket(PF_KEY, SOCK_RAW, PF_KEY_V2)) < 0) { > __ipsec_set_strerror(strerror(errno)); > @@ -1794,14 +1796,27 @@ pfkey_open(void) > * This is a temporary workaround for KAME PR 154. > * Don't really care even if it fails. > */ > - (void)setsockopt(so, SOL_SOCKET, SO_SNDBUF, &bufsiz, sizeof(bufsiz)); > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > - bufsiz = 256 * 1024; > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > - bufsiz = 512 * 1024; > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > - bufsiz = 1024 * 1024; > - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF, &bufsiz, sizeof(bufsiz)); > + /* Try to have 128k. If we have more, do not lower it. */ > + bufsiz_wanted = 128 * 1024; > + ret = getsockopt(so, SOL_SOCKET, SO_SNDBUF, > + &bufsiz_current, &len); len needs to be initialised to sizeof(bufsiz_current) before the getsockopt() call. Did this code ever work? I guess it works if the len gets value >=4 from stack. > + if ((ret < 0) || (bufsiz_current < bufsiz_wanted)) > + (void)setsockopt(so, SOL_SOCKET, SO_SNDBUF, > + &bufsiz_wanted, sizeof(bufsiz_wanted)); > + > + /* Try to have have at least 2MB. If we have more, do not lower it. */ > + bufsiz_wanted = 2 * 1024 * 1024; > + ret = getsockopt(so, SOL_SOCKET, SO_RCVBUF, > + &bufsiz_current, &len); And technically, len should be reinitialized before the second getsockopt() call too. > + if (ret < 0) > + bufsiz_current = 128 * 1024; > + > + for (; bufsiz_wanted > bufsiz_current; bufsiz_wanted /= 2) { > + if (setsockopt(so, SOL_SOCKET, SO_RCVBUF, > + &bufsiz_wanted, sizeof(bufsiz_wanted)) == 0) > + break; > + } > + > __ipsec_errcode = EIPSEC_NO_ERROR; > return so; > } This looks great now. I can do the final edit, and commit. Will do that in a minute. Thanks, Timo |
From: Marcelo L. <mle...@re...> - 2011-11-16 20:09:55
|
On 11/14/2011 11:14 AM, Timo Teräs wrote: > On 11/14/2011 03:01 PM, Marcelo Leitner wrote: >> Here goes the 3rd version. Please see what you think. We can update it >> again if needed, no problem. >> >> Thanks for all your patience and time, Timo. >> diff -up ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig ipsec-tools-0.8.0/src/libipsec/pfkey.c >> --- ipsec-tools-0.8.0/src/libipsec/pfkey.c.orig 2011-10-04 11:48:43.534727761 -0300 >> +++ ipsec-tools-0.8.0/src/libipsec/pfkey.c 2011-11-14 10:40:42.555775797 -0200 >> @@ -1783,7 +1783,9 @@ int >> pfkey_open(void) >> { >> int so; >> - int bufsiz = 128 * 1024; /*is 128K enough?*/ >> + int bufsiz_current, bufsiz_wanted; >> + int ret; >> + socklen_t len; >> >> if ((so = socket(PF_KEY, SOCK_RAW, PF_KEY_V2))< 0) { >> __ipsec_set_strerror(strerror(errno)); >> @@ -1794,14 +1796,27 @@ pfkey_open(void) >> * This is a temporary workaround for KAME PR 154. >> * Don't really care even if it fails. >> */ >> - (void)setsockopt(so, SOL_SOCKET, SO_SNDBUF,&bufsiz, sizeof(bufsiz)); >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> - bufsiz = 256 * 1024; >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> - bufsiz = 512 * 1024; >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> - bufsiz = 1024 * 1024; >> - (void)setsockopt(so, SOL_SOCKET, SO_RCVBUF,&bufsiz, sizeof(bufsiz)); >> + /* Try to have 128k. If we have more, do not lower it. */ >> + bufsiz_wanted = 128 * 1024; >> + ret = getsockopt(so, SOL_SOCKET, SO_SNDBUF, >> + &bufsiz_current,&len); > len needs to be initialised to sizeof(bufsiz_current) before the > getsockopt() call. Did this code ever work? I guess it works if the len > gets value>=4 from stack. > >> + if ((ret< 0) || (bufsiz_current< bufsiz_wanted)) >> + (void)setsockopt(so, SOL_SOCKET, SO_SNDBUF, >> + &bufsiz_wanted, sizeof(bufsiz_wanted)); >> + >> + /* Try to have have at least 2MB. If we have more, do not lower it. */ >> + bufsiz_wanted = 2 * 1024 * 1024; >> + ret = getsockopt(so, SOL_SOCKET, SO_RCVBUF, >> + &bufsiz_current,&len); > And technically, len should be reinitialized before the second > getsockopt() call too. > >> + if (ret< 0) >> + bufsiz_current = 128 * 1024; >> + >> + for (; bufsiz_wanted> bufsiz_current; bufsiz_wanted /= 2) { >> + if (setsockopt(so, SOL_SOCKET, SO_RCVBUF, >> + &bufsiz_wanted, sizeof(bufsiz_wanted)) == 0) >> + break; >> + } >> + >> __ipsec_errcode = EIPSEC_NO_ERROR; >> return so; >> } > This looks great now. > > I can do the final edit, and commit. Will do that in a minute. Great! Many thanks! Is there any commits mailing list with public archives? I couldn't find a reference to it. Thanks, Marcelo. |
From: Timo T. <tim...@ik...> - 2011-11-17 06:06:45
|
On 11/16/2011 10:09 PM, Marcelo Leitner wrote: > On 11/14/2011 11:14 AM, Timo Teräs wrote: >> This looks great now. >> >> I can do the final edit, and commit. Will do that in a minute. > > Great! Many thanks! > > Is there any commits mailing list with public archives? I couldn't find > a reference to it. The full commit diff is not. But the commit message + cvs rdiff commands to generate the diff are at: http://mail-index.netbsd.org/source-changes/ The specific messages for this commit are: http://mail-index.netbsd.org/source-changes/2011/11/14/msg028900.html http://mail-index.netbsd.org/source-changes/2011/11/14/msg028901.html - Timo |