Re: [ckrm-tech] [PATCH 6/6] Resource Groups over generic containers

Re: [ckrm-tech] [PATCH 6/6] Resource Groups over generic containers

[Balbir Singh]

Hi, Paul,

I've just started playing with the new patchset and found a few issues.

menage@google.com wrote:
> +ssize_t res_group_file_write(struct container *cont,
> +				   struct cftype *cft,
> +				   struct file *file,
> +				   const char __user *userbuf,
> +				   size_t nbytes, loff_t *ppos)
> +{
> +	struct res_group_cft *rgcft = container_of(cft, struct res_group_cft, cft);
> +	struct res_controller *ctlr = rgcft->ctlr;
> +
> +	if (nbytes >= PAGE_SIZE)
> +		return -E2BIG;
> +
> +	char *buf;
> +	ssize_t retval;
> +	int filetype = cft->private;
> +
> +	buf = kmalloc(nbytes + 1, GFP_USER);

This should be kmalloc(nbytes), an echo ".." has a "\n" associated
with it.

> +	if (!buf) return -ENOMEM;
> +	if (copy_from_user(buf, userbuf, nbytes)) {
> +		retval = -EFAULT;
> +		goto out1;
> +	}
> +	buf[nbytes] = 0;	/* nul-terminate */
> +

this should be buf[nbytes - 1]



-- 

	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Re: [ckrm-tech] [PATCH 6/6] Resource Groups over generic containers

[Paul Jackson]

Balbir wrote:
> This should be kmalloc(nbytes), an echo ".." has a "\n" associated
> with it.

But a:
  write(1, "..", 2);
does not have a trialing newline.

If some consumer of this kernel buffer copy of what the
user wrote cannot handle the possible trailing whitespace,
they will have to chomp (Perl phrase) it off.  You can't
just whack one byte blindly.

At least for the kernel/cpuset.c code, from whence this
came, the consumers of this kernel buffer copy are such
routines as simple_strtoul() and cpulist_parse(), both
of which cope with trailing newlines.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [ckrm-tech] [PATCH 6/6] Resource Groups over generic containers

[Balbir Singh]

Paul Jackson wrote:
> Balbir wrote:
>> This should be kmalloc(nbytes), an echo ".." has a "\n" associated
>> with it.
> 
> But a:
>   write(1, "..", 2);
> does not have a trialing newline.

Yes, true.

> 
> If some consumer of this kernel buffer copy of what the
> user wrote cannot handle the possible trailing whitespace,
> they will have to chomp (Perl phrase) it off.  You can't
> just whack one byte blindly.
> 

Yes, agreed.

> At least for the kernel/cpuset.c code, from whence this
> came, the consumers of this kernel buffer copy are such
> routines as simple_strtoul() and cpulist_parse(), both
> of which cope with trailing newlines.
> 

The problem I have is that match_token() that's used by
the resource group's infrastructure cannot deal with
"\n". I think the code needs in res_groups needs to
get smarter like the code in simple_strtoul()


-- 
	Thanks for the feedback,
	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Re: [ckrm-tech] [PATCH 6/6] Resource Groups over generic containers

[Balbir Singh]

Paul Jackson wrote:
> Balbir wrote:
>> This should be kmalloc(nbytes), an echo ".." has a "\n" associated
>> with it.
> 
> But a:
>   write(1, "..", 2);
> does not have a trialing newline.

Yes, true.

> 
> If some consumer of this kernel buffer copy of what the
> user wrote cannot handle the possible trailing whitespace,
> they will have to chomp (Perl phrase) it off.  You can't
> just whack one byte blindly.
> 

Yes, agreed.

> At least for the kernel/cpuset.c code, from whence this
> came, the consumers of this kernel buffer copy are such
> routines as simple_strtoul() and cpulist_parse(), both
> of which cope with trailing newlines.
> 

The problem I have is that match_token() that's used by
the resource group's infrastructure cannot deal with
"\n". I think the code needs in res_groups needs to
get smarter like the code in simple_strtoul()


-- 
	Thanks for the feedback,
	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs