Re: [PATCH] making /proc//limits writable

Jiri Slaby
2008-07-30T21:43:32Z

On 07/30/2008 09:57 PM, Anders Johansson wrote:
> (For any replies, please cc. me, because I'm not subscribed)
>
> Attached is a patch making the limits for a process writable, so it is
> possible to change the limits of a task other than current.
>
> There are many reasons why this is desirable. The core limit is to me the most
> important. If a process hangs, because of some sort of race condition that
> happens once in a blue moon, and ulimit -c is set to 0, it might take forever
> to reproduce. It would be nice to be able to change that limit dynamically, to
> be able to get a core dump.
>
> Other limits should also be settable, but for now I've only done core size.
>
> The patch is against 2.6.27-rc1
>
> btw, in case it isn't painfully obvious: this is my very first kernel patch
> that isn't a simple one-or-two-line bug fix

diff -ur a/fs/proc/base.c b/fs/proc/base.c
--- a/fs/proc/base.c 2008-07-30 21:44:44.000000000 +0200
+++ b/fs/proc/base.c 2008-07-30 21:53:07.000000000 +0200
@@ -423,6 +423,76 @@

#endif

+static ssize_t pid_limits_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *offs)
+{
+ int ret = 0;
+ struct task_struct *task;
+
+ if (!count)
+ goto out_no_task;

Mixing decls with code causes warnings.

+ char c;
+ char *s = buf, *tmp;

This is sparse unfriendly. Try to build with C=1. This must have generate a
compiler warning too (about the const).

+ char buffer[256];
+ unsigned long softlim, hardlim;
+ char hardlim_set = 0;
+
+ task = get_proc_task(file->f_dentry->d_inode);
+ if (!task) {
+ ret = -ESRCH;
+ goto out_no_task;
+ }
+ if (get_user(c, s)) {
+ ret = -EFAULT;
+ goto out;
+ }
+ s += 2;
+ switch (c) {
+ case 'c': {

I think this indent is not scripts/checkpatch.pl compliant as suggested by some
people already.

+ if (copy_from_user(buffer, s, 255)) {

Hmm, does this work? It should return unable-to-copy bytes which would mean to
return 255-count-2. Some kind of min(count-2, 255U); is needed here, I think.

+ ret = -EFAULT;
+ goto out;
+ }
+ if (strncmp(buffer, "unlimited", 9) == 0)
+ softlim = RLIM_INFINITY;
+ else {
+ softlim = strict_strtoul(buffer, &tmp, 10);

This doesn't compile, I suppose? Anyway you need buffer[255] = '\0'; somewhere
before this.

+ if (tmp == buffer) {
+ ret = -EFAULT;
+ goto out;
+ }
+ softlim *= 1024;
+ }
+ if ((tmp - buffer) < count) {

"Use of unitialized value" here? tmp & unlimited path.