lkml.org 
[lkml]   [2006]   [Aug]   [20]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateSun, 20 Aug 2006 10:26:02 +0200
FromWilly Tarreau <>
SubjectRe: [PATCH] set*uid() must not fail-and-return on OOM/rlimits
On Sun, Aug 20, 2006 at 04:38:40AM +0400, Solar Designer wrote:
> Willy and all,> > Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> set*uid() kill the current process rather than proceed with -EAGAIN when
> the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> and return anyway due to properties of the allocator, in which case the
> patch does not change a thing.  But better safe than sorry.

Whether it can fail or not, alloc_uid()'s author intent was to report its
problems via NULL :

                new = kmem_cache_alloc(uid_cachep, SLAB_KERNEL);
                if (!new)
                        return NULL;
So your change to set_user() are consistent with this design choice.
Now, chosing to kill the process whe the kernel runs out of memory
seems consistent with what will happen a few milliseconds later to
other processes anyway.

I'm just wondering why you return a SIGSEGV. When the kernel kills
tasks on OOM conditions, it sends either SIGTERM or SIGKILL, as we
can see here in mm/oom_kill.c:__oom_kill_task() :

        p->flags |= PF_MEMALLOC | PF_MEMDIE;
        /* This process has hardware access, be more careful. */
        if (cap_t(p->cap_effective) & CAP_TO_MASK(CAP_SYS_RAWIO)) {
                force_sig(SIGTERM, p);
        } else {
                force_sig(SIGKILL, p);
        }
Shouldn't we simply re-use the same code ? (not the function, I would not
like to get OOM messages outside the OOM killer).

> As you're probably aware, 2.6 kernels are affected to a greater extent,
> where set*uid() may also fail on trying to exceed RLIMIT_NPROC.  That
> needs to be fixed, too.

I've followed the thread a little bit but am not aware of all the details.

> Opinions are welcome.> > Thanks,> > Alexander

What do you (and others) think about this ?
Willy


> diff -urpPX nopatch linux-2.4.33/kernel/sys.c linux/kernel/sys.c
> --- linux-2.4.33/kernel/sys.c	Fri Nov 28 21:26:21 2003
> +++ linux/kernel/sys.c	Wed Aug 16 05:19:21 2006> @@ -514,8 +514,10 @@ static int set_user(uid_t new_ruid, int 
>  	struct user_struct *new_user;
> 
>  	new_user = alloc_uid(new_ruid);
> -	if (!new_user)
> +	if (!new_user) {
> +		force_sig(SIGSEGV, current);
>  		return -EAGAIN;
> +	}
>  	switch_uid(new_user);
> 
>  	if(dumpclear)

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-08-20 10:29    [from the cache]
©2003-2008