lkml.org 
[lkml]   [2008]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.
    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".




    \
     
     \ /
      Last update: 2008-11-10 11:37    [W:0.030 / U:30.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site