linux-kernel - Re: [PATCH v2 2/3] char drivers: ramoops record

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]
Message-ID: <BANLkTi=HqKggFkcLgN22qgTr5drz3_5vO+Q9bM+NqsOYVK5uag@mail.gmail.com>
Date:	Fri, 1 Jul 2011 11:41:11 -0700
From:	Sergiu Iordache <sergiu@...gle.com>
To:	Marco Stornelli <marco.stornelli@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"Ahmed S. Darwish" <darwish.07@...il.com>,
	Artem Bityutskiy <Artem.Bityutskiy@...ia.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter

On Fri, Jul 1, 2011 at 10:57 AM, Marco Stornelli
<marco.stornelli@...il.com> wrote:
> Hi,
>
> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>
>> The size of the dump is currently set using the RECORD_SIZE macro which
>> is set to a page size. This patch makes the record size a module
>> parameter and allows it to be set through platform data as well to allow
>> larger dumps if needed.
>>
>> Signed-off-by: Sergiu Iordache<sergiu@...omium.org>
>> Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
>> ---
>> The patch was built on the 2.6.38 kernel and is based on the following
>> patches which were applied from the mmotm tree:
>> ramoops-add-new-line-to-each-print
>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>
>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>
>>  drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
>>  include/linux/ramoops.h |    1 +
>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>> index 5349d94..f34077e 100644
>> --- a/drivers/char/ramoops.c
>> +++ b/drivers/char/ramoops.c
>> @@ -32,8 +32,12 @@
>>  #include<linux/ramoops.h>
>>
>>  #define RAMOOPS_KERNMSG_HDR "===="
>> +#define MIN_MEM_SIZE 4096UL
>>
>> -#define RECORD_SIZE 4096UL
>> +static ulong record_size = 4096UL;
>> +module_param(record_size, ulong, 0400);
>> +MODULE_PARM_DESC(record_size,
>> +               "size of each dump done on oops/panic");
>>
>>  static ulong mem_address;
>>  module_param(mem_address, ulong, 0400);
>> @@ -55,6 +59,7 @@ static struct ramoops_context {
>>        void *virt_addr;
>>        phys_addr_t phys_addr;
>>        unsigned long size;
>> +       unsigned long record_size;
>>        int dump_oops;
>>        int count;
>>        int max_count;
>> @@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper
>> *dumper,
>>        if (reason == KMSG_DUMP_OOPS&&  !cxt->dump_oops)
>>                return;
>>
>> -       buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
>> +       buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>        buf_orig = buf;
>>
>> -       memset(buf, '\0', RECORD_SIZE);
>> +       memset(buf, '\0', cxt->record_size);
>>        res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>>        buf += res;
>>        do_gettimeofday(&timestamp);
>> @@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
>>        buf += res;
>>
>>        hdr_size = buf - buf_orig;
>> -       l2_cpy = min(l2, RECORD_SIZE - hdr_size);
>> -       l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
>> +       l2_cpy = min(l2, cxt->record_size - hdr_size);
>> +       l1_cpy = min(l1, cxt->record_size - hdr_size - l2_cpy);
>>
>>        s2_start = l2 - l2_cpy;
>>        s1_start = l1 - l1_cpy;
>> @@ -119,16 +124,29 @@ static int __init ramoops_probe(struct
>> platform_device *pdev)
>>        }
>>
>>        rounddown_pow_of_two(pdata->mem_size);
>> +       rounddown_pow_of_two(pdata->record_size);
>>
>> -       if (pdata->mem_size<  RECORD_SIZE) {
>> +       /* Check for the minimum memory size */
>> +       if (pdata->mem_size<  MIN_MEM_SIZE) {
>> +               pr_err("memory size too small, min %lu\n", MIN_MEM_SIZE);
>> +               goto fail3;
>> +       }
>> +
>> +       if (pdata->mem_size<  pdata->record_size) {
>>                pr_err("size too small\n");
>>                goto fail3;
>>        }
>>
>> -       cxt->max_count = pdata->mem_size / RECORD_SIZE;
>> +       if (pdata->record_size<= 0) {
>> +               pr_err("record size too small\n");
>> +               goto fail3;
>> +       }
>
> There is something wrong here. First of all if record_size is unsigned it
> can't negative. In addition, if we are here, we know that:
>
> record_size >= mem_size >= MIN_MEM_SIZE
>
> So this check have no sense.

The pdata->record size <= 0 check is indeed redundant and should be removed.

I didn't completely understand the second comment, the module errors
if mem_size < MIN_MEM_SIZE or mem_size < record_size, which means that
mem_size should be larger than MIN_MEM_SIZE and record_size (which
leads to record_size being between 0 and mem_size). Am I missing
something?

(Resent after not reply-ing to all by mistake)

Sergiu
--
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/