Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname managementfunctions.

From: Kentaro Takeda
Date: Mon Nov 10 2008 - 05:34:33 EST


Andrew Morton wrote:
> > +/**
> > + * round_up - Round up an integer so that the returned pointers are appropriately aligned.
> > + *
> > + * @size: Size in bytes.
> > + *
> > + * Returns rounded value of @size.
> > + *
> > + * FIXME: Are there more requirements that is needed for assigning value
> > + * atomically?
> > + */
>
> Can PTR_ALIGN be used?
>
> If not, please prefer to avoid implementing generic helpers down in
> specific code. It is better to add the helpers in a kernel-wide
> fashion in an early patch, then to use those halpers in the
> subsyste-specific patches.

Replaced by "roundup(size, max(sizeof(void *), sizeof(long)))".

> > +/* Structure for string data. */
> > +struct name_entry {
> > + struct list1_head list;
> > + struct path_info entry;
> > +};
> > +
>
> You didn't need to invent list1_head for this application. This is
> *exactly* what the existing hlist_head is designed for.

hlist_head omits ->pprev, but hlist_node doesn't.
Since TOMOYO uses this list as Write-Once-Read-Many,
TOMOYO doesn't use ->pprev for list elements.

> > +/**
> > + * tmy_save_name - Allocate permanent memory for string data.
> > + *
> > + * @name: The string to store into the permernent memory.
> > + *
> > + * Returns pointer to "struct path_info" on success, NULL otherwise.
> > + *
> > + * The RAM is shared, so NEVER try to modify or kfree() the returned name.
> > + */
>
> Nothing ever gets removed from fmb_list. How odd.
>
> If this is not a bug, I'd suggest that a code comment be added
> explaining what all this code does and why it does it and how it does
> it.

fmb contains memory reserved by TOMOYO for future requests.
fmb is removed from the fmb_list when fmb->len becomes 0.
So, this is not a bug. I added a comment.

> > +/* Memory allocated for temporal purpose. */
> > +static atomic_t dynamic_memory_size;
>
> The correct word is "temporary". This needs fixing in at least one
> other place.

Replaced "temporal" by "temporary". Thanks.

> Is this counter really useful? If not, I'd suggest that it be removed
> and that all calls to tmy_alloc() simply be replaced by calls to
> kmalloc().
>
This counter was introduced by user's request so that the administrator can
know how much memory is used by TOMOYO module. So, I want to keep this counter.

> A better way to perform memory accounting would be to create slab
> caches for commonly-used objects and to reply uponthe existing
> accounting in /proc/slabinfo.
>
Hmm, memory allocated for temporary purpose is not a fixed size.

> > +/**
> > + * tmy_alloc - Allocate memory for temporal purpose.
> > + *
> > + * @size: Size in bytes.
> > + *
> > + * Returns pointer to allocated memory on success, NULL otherwise.
> > + */
> > +void *tmy_alloc(const size_t size)
> > +{
> > + void *p = kzalloc(size, GFP_KERNEL);
> > + if (p)
> > + atomic_add(ksize(p), &dynamic_memory_size);
> > + return p;
> > +}
>
> Note that I said "kmalloc", not "kzalloc". This function zeroes
> everything all the time, and surely that is not necessary. It's just a
> waste of CPU time.
>
Callers of tmy_alloc assume that allocated memory is zeroed.

> > +static int tmy_print_ascii(const char *sp, const char *cp,
> > + int *buflen0, char **end0)
>
> I look at this and wonder "hm, does that duplicate any facility which
> the kernel provides"? But I can't tell, because I don't know what this
> function does, and I shouldn't have to sit down with a pencil and paper
> decrypting it.
>
I splitted this function into "prepend()" part and "convert a string to
TOMOYO's string representation rule" part. And I renamed the latter from
"tmy_print_ascii" to "tmy_encode".

> > +/* tmy_realpath_from_path2() for "struct ctl_table". */
> > +static int tmy_sysctl_path(struct ctl_table *table, char *buffer, int buflen)
>
> Is this needed if CONFIG_SYSCTL=n? Does it compile if CONFIG_SYSCTL=n?
>
Added "#ifdef CONFIG_SYSCTL" and moved to security/tomoyo/tomoyo.c .

> > +/**
> > + * tmy_read_memory_counter - Check for memory usage.
> > + *
> > + * @head: Pointer to "struct tmy_io_buffer".
> > + *
> > + * Returns memory usage.
>
> In what units? Megabytes?
>
In bytes.

> > +int tmy_read_memory_counter(struct tmy_io_buffer *head)
>
> This (I assume) is part of an implementation of a userspace interface.
> We care a lot about userspace interfaces. Please describe the Tomoyo
> userspace interfaces so that we can review them for suitability and
> maintainability.
>
I'll describe it in other posting.

> Surely this function should return a 64-bit quantity?
>
I believe 32-bit is enough. TOMOYO uses only 1MB or so. Never 4GB or more.

> Again, we would like to see a complete decription of the proposed
> userspace ABI. This one looks fairly ugly. Do I really have to write
> 'S' 'h' 'a' 'r' 'e' 'd' ':' ' ' into some pseudo file?
>
> A better interface would be two suitably-named pseudo files each of
> which takes a bare integer string. None of this funny colon-based
> prefixing stuff.
>
Creating pseudo files for each variables is fine, though I don't see
advantage by changing from
"echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
"echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".


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