Skip to contentSkip to conversation

setrlimit incorrectly allows hard limits to exceed soft limits

TRIVIAL Rusty Trivial Russell 3 participants 4 messages Dec 6, 2002 – Dec 9, 2002

lore
Share & tools
lore
#0 lore raw

From: Kingsley Cheung <[email protected]>

Oops, should be after the copy :-(

Quoted 10 lines

In 2.4.19 (also 2.5.46) setrlimit code only ever makes a comparison to check the old soft limit with the new soft limit and the new hard limit with the old hard limit. There is never a check to ensure the new soft limit never exceeds the new hard limit.

Just try "ulimit -H -m 10000" for memory limits that were not previously set. You end up with (hard limit = 10000) < (soft limit = unlimited).

Fix is trivial.

kernel/sys.c renamed from kernel/sys.c.orig +2 -0
@@ -1233,6 +1233,8 @@
 		return -EINVAL;
 	if(copy_from_user(&new_rlim, rlim, sizeof(*rlim)))
 		return -EFAULT;
+       if (new_rlim.rlim_cur > new_rlim.rlim_max)
+               return -EINVAL;
 	old_rlim = current->rlim + resource;
 	if (((new_rlim.rlim_cur > old_rlim->rlim_max) ||
 	     (new_rlim.rlim_max > old_rlim->rlim_max)) &&
Signature · 2 lines
  Don't blame me: the Monkey is driving
  File: Kingsley Cheung <[email protected]>: Re: [PATCH] setrlimit incorrectly allows hard limits to exceed soft limits
#1 lore raw

kernel/sys.c line 1237

Quoted 6 lines · Rusty Trivial Russell
Quoted 3 lines

Just try "ulimit -H -m 10000" for memory limits that were not previously set. You end up with (hard limit = 10000) < (soft limit = unlimited).

kernel/sys.c renamed from kernel/sys.c.orig +2 -0
+       if (new_rlim.rlim_cur > new_rlim.rlim_max)
+               return -EINVAL;

Wouldn't it be better to simply take the soft limit down to min(new_rlim.rlim_cur, new_rlim.rlim_max) ?

regards,

Rik

Signature · 3 lines
A: No.
Q: Should I include quotations after my reply?
http://www.surriel.com/		http://guru.conectiva.com/
#2 lore raw
Quoted 11 lines · Rik van Riel

kernel/sys.c line 1237

Quoted 6 lines · Rusty Trivial Russell
Quoted 3 lines

Just try "ulimit -H -m 10000" for memory limits that were not previously set. You end up with (hard limit = 10000) < (soft limit = unlimited).

kernel/sys.c renamed from kernel/sys.c.orig +2 -0
+       if (new_rlim.rlim_cur > new_rlim.rlim_max)
+               return -EINVAL;

Wouldn't it be better to simply take the soft limit down to min(new_rlim.rlim_cur, new_rlim.rlim_max) ?

Another way to do it would be to adjust the max limit up to the soft limit only if the process had CAP_SYS_RESOURCE, just like in the function svr4_ulimit in abi/svr4/ulimit.c.

In both cases, however, changing it will mean changing a limit on behalf of the process. But IMHO wouldn't most applications would expect rlimits modified with setrlimit successfully to be set to the rlimits that they specified as the argument? Any child processes they spawn would expect the same set of modified rlimits to be inherited as well. If we fix an incorrect pair of rlimits on their behalf, they would then see different rlimit values with getrlimit later.

In light of that wouldn't it be better just to return an error and insist that broken applications get fixed? Unless of course there are just too many broken apps out there : )

--
Kingsley

#3 lore raw
In reply to Rik van Riel

In message <Pine.LNX.4.50L.0212061106050.22252-100000@duckman.distro.conectiva>
you write:

Quoted 11 lines · Rusty Trivial Russell

kernel/sys.c line 1237

Quoted 7 lines
Quoted 4 lines

Just try "ulimit -H -m 10000" for memory limits that were not previously set. You end up with (hard limit = 10000) < (soft limit = unlimited).

kernel/sys.c renamed from kernel/sys.c.orig +2 -0
+       if (new_rlim.rlim_cur > new_rlim.rlim_max)
+               return -EINVAL;

Wouldn't it be better to simply take the soft limit down to min(new_rlim.rlim_cur, new_rlim.rlim_max) ?

POSIX says -EINVAL, and since it's a programmer fuckup, I'd agree.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.