svc_check_conn_limits() attempts to prevent denial of service attacks
by having the service close old connections once it reaches a
threshold. This threshold is based on the number of threads in the
service:
(serv->sv_nrthreads + 3) * 20
Once we reach this, we drop the oldest connections and a printk pops
to warn the admin that they should increase the number of threads.
Increasing the number of threads isn't an option however for services
like lockd. We don't want to eliminate this check entirely for such
services but we need some way to increase this limit.
This patch adds a sv_maxconn field to the svc_serv struct. When it's
set to 0, we use the current method to calculate the max number of
connections. RPC services can then set this on an as-needed basis.
Signed-off-by: Jeff Layton <[email protected]>
---
include/linux/sunrpc/svc.h | 1 +
net/sunrpc/svc_xprt.c | 16 ++++++++++++----
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 3afe7fb..048da8a 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -61,6 +61,7 @@ struct svc_serv {
unsigned int sv_max_payload; /* datagram payload size */
unsigned int sv_max_mesg; /* max_payload + 1 page for overheads */
unsigned int sv_xdrsize; /* XDR buffer size */
+ unsigned int sv_maxconn; /* max connections allowed */
struct list_head sv_permsocks; /* all permanent sockets */
struct list_head sv_tempsocks; /* all temporary sockets */
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index bf5b5cd..264f138 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -525,19 +525,27 @@ int svc_port_is_privileged(struct sockaddr *sin)
* The only somewhat efficient mechanism would be if drop old
* connections from the same IP first. But right now we don't even
* record the client IP in svc_sock.
+ *
+ * single-threaded services that expect a lot of clients will probably
+ * need to set sv_maxconn to override the default value which is based
+ * on the number of threads
*/
static void svc_check_conn_limits(struct svc_serv *serv)
{
- if (serv->sv_tmpcnt > (serv->sv_nrthreads+3)*20) {
+ unsigned int limit = serv->sv_maxconn ? serv->sv_maxconn :
+ (serv->sv_nrthreads+3) * 20;
+
+ if (serv->sv_tmpcnt > limit) {
struct svc_xprt *xprt = NULL;
spin_lock_bh(&serv->sv_lock);
if (!list_empty(&serv->sv_tempsocks)) {
if (net_ratelimit()) {
/* Try to help the admin */
printk(KERN_NOTICE "%s: too many open "
- "connections, consider increasing the "
- "number of nfsd threads\n",
- serv->sv_name);
+ "connections, consider increasing %s\n",
+ serv->sv_name, serv->sv_maxconn ?
+ "the max number of connections." :
+ "the number of threads.");
}
/*
* Always select the oldest connection. It's not fair,
--
1.5.5.1
The default method for calculating the number of connections allowed
per RPC service arbitrarily limits single-threaded services to 80
connections. This is too low for services like lockd and artificially
limits the number of TCP clients that it can support.
Have lockd set a default sv_maxconn value to 1024 (which is the typical
default value for RLIMIT_NOFILE. Also add a module parameter to allow an
admin to set this to an arbitrary value.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/lockd/svc.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index c631a83..e804155 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -62,6 +62,9 @@ static unsigned long nlm_timeout = LOCKD_DFLT_TIMEO;
static int nlm_udpport, nlm_tcpport;
int nsm_use_hostnames = 0;
+/* RLIM_NOFILE defaults to 1024. That seems like a reasonable default here. */
+static unsigned int nlm_max_connections = 1024;
+
/*
* Constants needed for the sysctl interface.
*/
@@ -143,6 +146,9 @@ lockd(void *vrqstp)
long timeout = MAX_SCHEDULE_TIMEOUT;
RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]);
+ /* update sv_maxconn if it has changed */
+ rqstp->rq_server->sv_maxconn = nlm_max_connections;
+
if (signalled()) {
flush_signals(current);
if (nlmsvc_ops) {
@@ -275,6 +281,7 @@ int lockd_up(void)
}
svc_sock_update_bufs(serv);
+ serv->sv_maxconn = nlm_max_connections;
nlmsvc_task = kthread_run(lockd, nlmsvc_rqst, serv->sv_name);
if (IS_ERR(nlmsvc_task)) {
@@ -484,6 +491,7 @@ module_param_call(nlm_udpport, param_set_port, param_get_int,
module_param_call(nlm_tcpport, param_set_port, param_get_int,
&nlm_tcpport, 0644);
module_param(nsm_use_hostnames, bool, 0644);
+module_param(nlm_max_connections, uint, 0644);
/*
* Initialising and terminating the module.
--
1.5.5.1
On Fri, 17 Oct 2008 17:18:33 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Fri, Oct 17, 2008 at 02:26:10PM -0400, Jeff Layton wrote:
> > The default method for calculating the number of connections allowed
> > per RPC service arbitrarily limits single-threaded services to 80
> > connections. This is too low for services like lockd and artificially
> > limits the number of TCP clients that it can support.
> >
> > Have lockd set a default sv_maxconn value to RLIMIT_NOFILE for the
> > lockd thread (usually this will be 1024). Also add a module parameter
> > to allow an admin to set this to an arbitrary value at module load
> > time.
>
> I guess this is OK.
>
> As long as we're picking a number out of thin air, I'd rather we make
> that obvious, instead of making it look like we made some kind of
> sophisticated choice that the poor reader will feel obliged to
> understand.
>
> So I'd be for a default that's just a constant, until someone has a
> better idea.
>
> What would actually happen if we allowed too many connections? What
> would fail first? Is there some way to detect that situation and use
> that to drop connections?
>
I've gone ahead and respun these patches. The main changes are:
- fixed the printk in svc_check_conn_limits()
- changed the default sv_maxconn to just use a constant value and added
a comment to clarify that it's basically arbitrary
- made it so that sv_maxconn is updated in lockd's main loop so this
value can be changed on the fly
Look OK?
--
Jeff Layton <[email protected]>
On Friday October 17, [email protected] wrote:
>
> I'm OK with that too. I just used this since Neil suggested it. I have
> no idea what a reasonable value should really be. I suppose this should
> probably be set to the maximum number of clients we expect to support
> (assuming 1 connection to lockd from each client).
I suggested it (using the NFILE rlimit) because I was looking for a
natural way to make it configurable, and asked myself "how would this
be handled by a user-space daemon". I managed to address one of those
questions, but not really the more useful one.
>
> > What would actually happen if we allowed too many connections? What
> > would fail first? Is there some way to detect that situation and use
> > that to drop connections?
> >
>
> I'm not clear on this either. Here's my naive take (could be very
> wrong):
I've got no really idea either.
But when we are building network-facing software, you need to be
cautious and use the "deny unless explicitly permitted" approached.
It may be safe to allow an arbitrarily large number of connections.
But until you can be certain of that you should fail-safe and impose a
limit.
NeilBrown
On Saturday October 18, [email protected] wrote:
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 3afe7fb..048da8a 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -61,6 +61,7 @@ struct svc_serv {
> unsigned int sv_max_payload; /* datagram payload size */
> unsigned int sv_max_mesg; /* max_payload + 1 page for overheads */
> unsigned int sv_xdrsize; /* XDR buffer size */
> + unsigned int sv_maxconn; /* max connections allowed
+ * or '0' causing max
+ * to be dynamic based on
+ * number of threads. */
Otherwise I am completely happy with these patches.
Thanks,
NeilBrown
On Mon, 20 Oct 2008 11:49:33 +1100
Neil Brown <[email protected]> wrote:
> On Friday October 17, [email protected] wrote:
> >
> > I'm OK with that too. I just used this since Neil suggested it. I have
> > no idea what a reasonable value should really be. I suppose this should
> > probably be set to the maximum number of clients we expect to support
> > (assuming 1 connection to lockd from each client).
>
> I suggested it (using the NFILE rlimit) because I was looking for a
> natural way to make it configurable, and asked myself "how would this
> be handled by a user-space daemon". I managed to address one of those
> questions, but not really the more useful one.
>
> >
> > > What would actually happen if we allowed too many connections? What
> > > would fail first? Is there some way to detect that situation and use
> > > that to drop connections?
> > >
> >
> > I'm not clear on this either. Here's my naive take (could be very
> > wrong):
>
> I've got no really idea either.
> But when we are building network-facing software, you need to be
> cautious and use the "deny unless explicitly permitted" approached.
> It may be safe to allow an arbitrarily large number of connections.
> But until you can be certain of that you should fail-safe and impose a
> limit.
>
For now, I think we should probably go with the patchset as-is (modulo
the comment change that Neil suggested), it at least addresses the
immediate problem of too many lockd connections.
In the long run, since it's not clear what we're preventing by doing
this, does it make sense to base the default sv_maxconn on the number
of threads? In principle, we could have a server that has 2000 clients,
but they each only rarely touch their NFS mount. In that case, we may
be able to get by with fewer nfsd's, but we'd want to increase the
number of allowed connections. The reverse is also true.
Perhaps we should just declare a constant sv_maxconn value for each
service, give each a way to tune it and use some other scheme (duty
cycle of nfsd's?) to warn the admin that they should consider increasing
the number of threads.
--
Jeff Layton <[email protected]>