linux-kernel - Re: [RFC] persistent store (version 3) (part 1 of 2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 8 Dec 2010 09:12:21 -0800
From:	Tony Luck <tony.luck@...el.com>
To:	Borislav Petkov <bp@...en8.de>, "Luck, Tony" <tony.luck@...el.com>,
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	tglx@...utronix.de, mingo@...e.hu, greg@...ah.com,
	akpm@...ux-foundation.org, ying.huang@...el.com,
	David Miller <davem@...emloft.net>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	Kyungmin Park <kmpark@...radead.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [RFC] persistent store (version 3) (part 1 of 2)

On Tue, Dec 7, 2010 at 10:24 PM, Borislav Petkov <bp@...en8.de> wrote:
> looks good. Minor nitpicks below.

Thanks for looking at this.

>> 1) Is "struct file" too big to be on the stack? I can change it to kmalloc() it.
>
> Well, this happens during normal operation when we init the pstore and
> since we don't need it after pstore_mkwrite has returned (do we?), I
> guess we should be fine.
struct file is 184 bytes (with the CONFIG options I am using, it can get
a bit larger, but not much).

>> +             the dying moments.  In the case of a panic() the last part
>
>                                                    "panic" (I'd remove the
>                                                                     brackets)

Ok. Will do.

>> +     dentry = d_alloc_name(root, name);
>> +     if (!IS_ERR(dentry))
>> +             d_add(dentry, inode);
>
> what happens if it IS_ERR? Error handling like
>
>        goto d_alloc_error;
>
> d_alloc_error:
>        iput(inode);
>        return -ENOSPC;
>
>
> or similar, at least this is what ramfs seems to be doing.

Good point - I need to clean up if things fail (and akpm would
like to see me re-factor so that there is only one "return"
statement for better maintainabiliy).  I'll fix up stuff here.

>> +     kmsg_dump_register(&pstore_dumper);
>
> You don't check psi->write() method's existence anymore, I'm assuming
> this is implied now... ?

Andrew's comments on version 2 were:
>It doesn't seem appropriate to check this here.  It's a programming
>error!  Just install the thing and let the kernel oops - the programmer
>will notice.

So I dropped the tests ... just checking whether the function pointer
was NULL wasn't a very strong test anyway.

>> +     if (!psinfo)
>> +             return -ENODEV;
>
> newline.

OK.

>> +#define PSTOREFS_MAGIC               0x6165676C
>
> what does that mean anyway? "aegl" :)

My initials: Anthony Eric George Luck. I've been using "aegl"
as my Unix login since 1979 (6th Edition on a pdp11/34).

>> +/* types */
>> +#define      PSTORE_TYPE_DMESG       0
>> +#define      PSTORE_TYPE_MCE         1
>
> You could make this into a proper enum
>
> enum pstore_type_id {
>        PSTORE_TYPE_DMESG       = 0,
>        PSTORE_TYPE_MCE         = 1,
>        PSTORE_TYPE_MAX,
> };
OK. Type checking is nice.  I think I might need a
PSTORE_TYPE_UNKNOWN to handle the case
where a new platform driver saves a record to
persistent store, and then the user boots an older
kernel that doesn't know about the new type (in
the APEI/ERST driver the type turns into a UUID
in the UEFI header for the record - so there is some
mapping going on at that level).

>> +struct pstore_info {
>> +     struct module   *owner;
>> +     char            *name;
>> +     struct mutex    mutex;  /* serialize access to 'buf' */
>
>  [ maybe a more descriptive variable name like buf_mutex or whatever ]

Sure. Will change.

>> +#if defined(CONFIG_PSTORE) || defined(CONFIG_PSTORE_MODULE)
>
> What is CONFIG_PSTORE_MODULE? Can't seem to find it in your (2 of 2)
> message either.

This is a remnant of when PSTORE was tristate - when I chose 'm'
rather than 'y' in "make menuconfig" I ended up with CONFIG_PSTORE_MODULE
rather than CONFIG_PSTORE.  But now it is just a "bool" this isn't
needed any more. Will fix.

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