[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <eb9bc944-b1de-48d9-652f-9f898ec4fcec@huawei.com>
Date: Wed, 14 Mar 2018 15:02:06 +0200
From: Igor Stoppa <igor.stoppa@...wei.com>
To: Matthew Wilcox <willy@...radead.org>
CC: <david@...morbit.com>, <rppt@...ux.vnet.ibm.com>, <keescook@...omium.org>,
<mhocko@...nel.org>, <labbott@...hat.com>,
<linux-security-module@...r.kernel.org>, <linux-mm@...ck.org>,
<linux-kernel@...r.kernel.org>, <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH 5/8] Protectable Memory
On 14/03/18 14:15, Matthew Wilcox wrote:
> On Tue, Mar 13, 2018 at 11:45:51PM +0200, Igor Stoppa wrote:
>> +static inline void *pmalloc_array(struct gen_pool *pool, size_t n,
>> + size_t size, gfp_t flags)
>> +{
>> + if (unlikely(!(pool && n && size)))
>> + return NULL;
>
> Why not use the same formula as kvmalloc_array here? You've failed to
> protect against integer overflow, which is the whole point of pmalloc_array.
>
> if (size != 0 && n > SIZE_MAX / size)
> return NULL;
oops :-(
>> +static inline char *pstrdup(struct gen_pool *pool, const char *s, gfp_t gfp)
>> +{
>> + size_t len;
>> + char *buf;
>> +
>> + if (unlikely(pool == NULL || s == NULL))
>> + return NULL;
>
> No, delete these checks. They'll mask real bugs.
I thought I got rid of all of them, but some have escaped me
>> +static inline void pfree(struct gen_pool *pool, const void *addr)
>> +{
>> + gen_pool_free(pool, (unsigned long)addr, 0);
>> +}
>
> It's poor form to use a different subsystem's type here. It ties you
> to genpool, so if somebody wants to replace it, you have to go through
> all the users and change them. If you use your own type, it's a much
> easier task.
I thought about it, but typedef came to my mind and knowing it's usually
frowned upon, I restrained myself.
> struct pmalloc_pool {
> struct gen_pool g;
> }
I didn't think this could be acceptable either. But if it is, then ok.
> then:
>
> static inline void pfree(struct pmalloc_pool *pool, const void *addr)
> {
> gen_pool_free(&pool->g, (unsigned long)addr, 0);
> }
>
> Looking further down, you could (should) move the contents of pmalloc_data
> into pmalloc_pool; that's one fewer object to keep track of.
>
>> +struct pmalloc_data {
>> + struct gen_pool *pool; /* Link back to the associated pool. */
>> + bool protected; /* Status of the pool: RO or RW. */
>> + struct kobj_attribute attr_protected; /* Sysfs attribute. */
>> + struct kobj_attribute attr_avail; /* Sysfs attribute. */
>> + struct kobj_attribute attr_size; /* Sysfs attribute. */
>> + struct kobj_attribute attr_chunks; /* Sysfs attribute. */
>> + struct kobject *pool_kobject;
>> + struct list_head node; /* list of pools */
>> +};
>
> sysfs attributes aren't free, you know. I appreciate you want something
> to help debug / analyse, but having one file for the whole subsystem or
> at least one per pool would be a better idea.
Which means that it should not be normal sysfs, but rather debugfs, if I
understand correctly, since in sysfs 1 value -> 1 file.
--
igor
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
