Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530594 > unrolled thread

Re: [RFC 1/1] LSM ptags: Add tagging of processes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 1/1] LSM ptags: Add tagging of processes Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-11-26 05:30 +0100
    Re: [RFC 1/1] LSM ptags: Add tagging of processes José Bollo <jobol@nonadev.net> - 2016-11-28 10:20 +0100
      Re: [RFC 1/1] LSM ptags: Add tagging of processes Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-11-28 14:50 +0100
        Re: [RFC 1/1] LSM ptags: Add tagging of processes José Bollo <jobol@nonadev.net> - 2016-11-29 00:50 +0100

#1530594 — Re: [RFC 1/1] LSM ptags: Add tagging of processes

Jose Bollo wrote:
> +/**
> + * is_valid_utf8 - Is buffer a valid utf8 string?
> + *
> + * @buffer: the start of the string
> + * @length: length in bytes of the buffer
> + *
> + * Return 1 when valid or else returns 0
> + */

Do we really need to check UTF-8 inside kernel? What do you do if
people start using UTF-32 in the future? There was a discussion
about use of encoding inside kernel started at
http://lkml.kernel.org/r/20071103164303.GA26707@ubuntu .



> +
> +/**
> + * _ptags_read - Implement the reading of the tags
> + *
> + * @ptags: tags structure of the readen task
> + * @result: a pointer for storing the read result
> + * @uns: user namespace proxy
> + *
> + * Returns the count of byte read or the negative code -ENOMEM
> + * if an allocation failed.
> + */
> +static int _ptags_read(struct _ptags *ptags, char **result, struct uns uns)
> +{
> +	unsigned idx, count;
> +	size_t size;
> +	struct entry *entries, *entry;
> +	char *buffer;
> +	struct value *value;
> +	struct item *item;
> +
> +	/* init loops */
> +	count = ptags->count;
> +	entries = ptags->entries;
> +
> +	/* compute printed size */
> +	size = 0;
> +	for (idx = 0; idx < count; idx++) {
> +		entry = &entries[idx];
> +		value = entry_read(entry, uns);
> +		if (value) {
> +			item = value_get(*value);
> +			size += entry_name(*entry)->length
> +				+ (unsigned)value_is_kept(*value)
> +				+ (item ? 2 + item->length : 1);
> +		}
> +	}
> +
> +	if (size > INT_MAX)
> +		return -E2BIG;

This sanity check is useless. INT_MAX is 2,147,483,647 but kmalloc() can't
allocate larger than 8,388,608 bytes if PAGE_SIZE = 4096 and MAX_ORDER = 11.

> +	buffer = kmalloc(size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;

Moreover, kmalloc() will not try to allocate larger than 32,768 bytes
(PAGE_ALLOC_COSTLY_ORDER = 3). Although __GFP_NOFAIL can force kmalloc() to
retry by invoking the OOM killer, such behavior might change in near future
due to http://lkml.kernel.org/r/20161123064925.9716-3-mhocko@kernel.org .

Given these constants

#define MAXCOUNT	4000
#define MAXTAGLEN	4000
#define MAXVALUELEN	32700

and someone tried to use as many and long as possible tags, what is
possible max value for "size"? I think that that value can easily exceed
32,768 and kmalloc() won't be reliable. vmalloc() can be used as a fallback
when kmalloc() failed, but is trying to pass possible max value for "size"
to vmalloc() reasonable? Shouldn't this function be rewritten not to
allocate so much memory?

Also, how much memory will be consumed if everybody tried to use tags
as many and long as possible?

> +
> +	/* print in the buffer */
> +	*result = buffer;
> +	for (idx = 0; idx < count; idx++) {
> +		entry = &entries[idx];
> +		value = entry_read(entry, uns);
> +		if (value) {
> +			if (value_is_kept(*value))
> +				*buffer++ = KEEP_CHAR;
> +			item = entry_name(*entry);
> +			memcpy(buffer, item->value, item->length);
> +			buffer += item->length;
> +			item = value_get(*value);
> +			if (item) {
> +				*buffer++ = ASSIGN_CHAR;
> +				memcpy(buffer, item->value, item->length);
> +				buffer += item->length;
> +			}
> +			*buffer++ = EOL_CHAR;
> +		}
> +	}
> +
> +	return (int)size;
> +}

[toc] | [next] | [standalone]

#1531140

Le samedi 26 novembre 2016 à 13:25 +0900, Tetsuo Handa a écrit :
> Jose Bollo wrote:
> > +/**
> > + * is_valid_utf8 - Is buffer a valid utf8 string?
snip
> Do we really need to check UTF-8 inside kernel? What do you do if
> people start using UTF-32 in the future? There was a discussion
> about use of encoding inside kernel started at
> http://lkml.kernel.org/r/20071103164303.GA26707@ubuntu .

Hello Tetsuo-san,

First, thank you for your bright review.

I followed that first thread a lot and conclude to remove that check in
favour of just prohibiting nul bytes that otherwise could create
problems.

snip
> > +
> > +	if (size > INT_MAX)
> > +		return -E2BIG;
> 
> This sanity check is useless. INT_MAX is 2,147,483,647 but kmalloc()
> can't
> allocate larger than 8,388,608 bytes if PAGE_SIZE = 4096 and
> MAX_ORDER = 11.
> 
> > +	buffer = kmalloc(size, GFP_KERNEL);
> > +	if (!buffer)
> > +		return -ENOMEM;
> 
> Moreover, kmalloc() will not try to allocate larger than 32,768 bytes
> (PAGE_ALLOC_COSTLY_ORDER = 3). Although __GFP_NOFAIL can force
> kmalloc() to
> retry by invoking the OOM killer, such behavior might change in near
> future
> due to http://lkml.kernel.org/r/20161123064925.9716-3-mhocko@kernel.o
> rg .
> 
> Given these constants
> 
> #define MAXCOUNT	4000
> #define MAXTAGLEN	4000
> #define MAXVALUELEN	32700
> 
> and someone tried to use as many and long as possible tags, what is
> possible max value for "size"? I think that that value can easily
> exceed
> 32,768 and kmalloc() won't be reliable. vmalloc() can be used as a
> fallback
> when kmalloc() failed, but is trying to pass possible max value for
> "size"
> to vmalloc() reasonable? Shouldn't this function be rewritten not to
> allocate so much memory?
> 
> Also, how much memory will be consumed if everybody tried to use tags
> as many and long as possible?

The fact is that ptags is seat behind the implementation of the special
files in /proc/PID/attr/.. Thus, it has to return an allocated buffer.
I'm not aware of what kind of allocation is possible to use for this
subsystem. Is it possible to use vmalloc? I dont know.

However, the remark is not just about how to implement it technically.
It is also about what amount of memory ptags could consume for threads.

My first naive idea is that consumers of ptags are reasonnables. Am I
fool, crazy or utopian? Guess...

I'm in favour of having a guard. And the gurad of a given memory size
seems good to my eyes. I could rewrite to enforce at most the use of
say 32700 characters. It could be enough for most interesting usage and
conversely it would fit the memory requirement that you pointed out.

Thanks again

Best regards
José

[toc] | [prev] | [next] | [standalone]

#1531307

Jose Bollo wrote:
> The fact is that ptags is seat behind the implementation of the special
> files in /proc/PID/attr/.. Thus, it has to return an allocated buffer.
> I'm not aware of what kind of allocation is possible to use for this
> subsystem. Is it possible to use vmalloc? I dont know.

If you replace kfree() with kvfree() in proc_pid_attr_read() in fs/proc/base.c ,
you will be able to use vmalloc(). However, proc_pid_attr_write() accepts only
PAGE_SIZE bytes from the beginning. If you want to use attributes longer than
PAGE_SIZE bytes, I think that you will need to use a different interface because
implementing I/O protocol for ptags which reads/writes PAGE_SIZE bytes chunk
at a time using /proc/PID/attr/ interface is not userspace friendly.

[toc] | [prev] | [next] | [standalone]

#1531807

Le lundi 28 novembre 2016 à 22:41 +0900, Tetsuo Handa a écrit :
> Jose Bollo wrote:
> > The fact is that ptags is seat behind the implementation of the
> > special
> > files in /proc/PID/attr/.. Thus, it has to return an allocated
> > buffer.
> > I'm not aware of what kind of allocation is possible to use for
> > this
> > subsystem. Is it possible to use vmalloc? I dont know.
> 
> If you replace kfree() with kvfree() in proc_pid_attr_read() in
> fs/proc/base.c ,
> you will be able to use vmalloc(). However, proc_pid_attr_write()
> accepts only
> PAGE_SIZE bytes from the beginning. If you want to use attributes
> longer than
> PAGE_SIZE bytes, I think that you will need to use a different
> interface because
> implementing I/O protocol for ptags which reads/writes PAGE_SIZE
> bytes chunk
> at a time using /proc/PID/attr/ interface is not userspace friendly.

Limiting a line of write to the length of PAGE_SIZE is ok for me.

Limiting the space of tags for a thread to 32700 is also ok for me.

It is safe, careful and sensible.

These limits will allow to advance in the use of ptags without
modifying much things.

Best regards
José

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web