Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966045AbcKKAM2 (ORCPT ); Thu, 10 Nov 2016 19:12:28 -0500 Received: from mail-wm0-f42.google.com ([74.125.82.42]:35478 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935555AbcKKAM0 (ORCPT ); Thu, 10 Nov 2016 19:12:26 -0500 MIME-Version: 1.0 In-Reply-To: <1476948846-15006-5-git-send-email-joelaf@google.com> References: <1476948846-15006-1-git-send-email-joelaf@google.com> <1476948846-15006-5-git-send-email-joelaf@google.com> From: Kees Cook Date: Thu, 10 Nov 2016 16:12:24 -0800 X-Google-Sender-Auth: TDFyiCvfQbAhWQAtnjNhvffCcpc Message-ID: Subject: Re: [PATCH v2 4/7] pstore: Make ramoops_init_przs generic for other prz arrays To: Joel Fernandes Cc: LKML , Anton Vorontsov , Colin Cross , Tony Luck Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5322 Lines: 141 On Thu, Oct 20, 2016 at 12:34 AM, Joel Fernandes wrote: > Currently ramoops_init_przs is hard wired only for panic dump zone array. In > preparation for the ftrace zone array (one zone per-cpu), make the function > more generic to be able to handle this case. > > Add a new ramoops_init_dump_przs function and use the generic function for the > dump prz array. > > Signed-off-by: Joel Fernandes > --- > fs/pstore/ram.c | 72 ++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 46 insertions(+), 26 deletions(-) > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > index 9104ae9..5caa512 100644 > --- a/fs/pstore/ram.c > +++ b/fs/pstore/ram.c > @@ -405,54 +405,74 @@ static void ramoops_free_przs(struct ramoops_context *cxt) > } > > static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt, > - phys_addr_t *paddr, size_t dump_mem_sz) > + struct persistent_ram_zone ***przs, > + phys_addr_t *paddr, size_t mem_sz, > + unsigned int cnt, u32 sig) > { > int err = -ENOMEM; > int i; > + size_t zone_sz; > + struct persistent_ram_zone **prz_ar; > > - if (!cxt->record_size) > - return 0; > - > - if (*paddr + dump_mem_sz - cxt->phys_addr > cxt->size) { > - dev_err(dev, "no room for dumps\n"); > + if (*paddr + mem_sz - cxt->phys_addr > cxt->size) { > + dev_err(dev, "no room for mem region (0x%zx@0x%llx) in (0x%lx@0x%llx)\n", > + mem_sz, (unsigned long long)*paddr, > + cxt->size, (unsigned long long)cxt->phys_addr); > return -ENOMEM; > } > > - cxt->max_dump_cnt = dump_mem_sz / cxt->record_size; > - if (!cxt->max_dump_cnt) > + zone_sz = mem_sz / cnt; > + if (!zone_sz) > return -ENOMEM; > > - cxt->przs = kzalloc(sizeof(*cxt->przs) * cxt->max_dump_cnt, > - GFP_KERNEL); > - if (!cxt->przs) { > - dev_err(dev, "failed to initialize a prz array for dumps\n"); > - goto fail_mem; > + prz_ar = kcalloc(cnt, sizeof(**przs), GFP_KERNEL); > + if (!prz_ar) { > + dev_err(dev, "Failed to initialize prz array\n"); > + return -ENOMEM; > } > > - for (i = 0; i < cxt->max_dump_cnt; i++) { > - cxt->przs[i] = persistent_ram_new(*paddr, cxt->record_size, 0, > + for (i = 0; i < cnt; i++) { > + prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig, > &cxt->ecc_info, > cxt->memtype); > - if (IS_ERR(cxt->przs[i])) { > - err = PTR_ERR(cxt->przs[i]); > + if (IS_ERR(prz_ar[i])) { > + err = PTR_ERR(prz_ar[i]); > dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n", > cxt->record_size, (unsigned long long)*paddr, err); > > while (i > 0) { > i--; > - persistent_ram_free(cxt->przs[i]); > + persistent_ram_free(prz_ar[i]); > } > - goto fail_prz; > + kfree(prz_ar); > + return err; > } > - *paddr += cxt->record_size; > + *paddr += zone_sz; > } > > + *przs = prz_ar; > return 0; > -fail_prz: > - kfree(cxt->przs); > -fail_mem: > - cxt->max_dump_cnt = 0; > - return err; > +} > + > +static int ramoops_init_dump_przs(struct device *dev, > + struct ramoops_context *cxt, > + phys_addr_t *paddr, size_t mem_sz) > +{ > + int ret; > + > + if (!cxt->record_size) > + return 0; > + > + cxt->max_dump_cnt = mem_sz / cxt->record_size; > + if (!cxt->max_dump_cnt) > + return -ENOMEM; > + > + ret = ramoops_init_przs(dev, cxt, &cxt->przs, paddr, mem_sz, > + cxt->max_dump_cnt, 0); > + if (ret) > + cxt->max_dump_cnt = 0; > + > + return ret; > } > > static int ramoops_init_prz(struct device *dev, struct ramoops_context *cxt, > @@ -604,7 +624,7 @@ static int ramoops_probe(struct platform_device *pdev) > > dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size > - cxt->pmsg_size; > - err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz); > + err = ramoops_init_dump_przs(dev, cxt, &paddr, dump_mem_sz); > if (err) > goto fail_out; > > -- > 2.7.4 > This looks good. One thing that I think needs to be adjusted is that the actual size of the allocation ("cnt") keeps being recalculated and passed around. It seems like the counts need to be managed for each prz array in the context. (i.e. dump_max_cnt exists, but not for the ftrace prz array). What do you think of adding that? -Kees -- Kees Cook Nexus Security