linux-kernel - Re: [PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory allocator

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:   Thu, 28 Sep 2023 20:23:36 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Jeff Johnson <quic_jjohnson@...cinc.com>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        kernel@...cinc.com,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory allocator

On Thu, Sep 28, 2023 at 8:19 PM Jeff Johnson <quic_jjohnson@...cinc.com> wrote:
>
> On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> > +void *qcom_scm_mem_alloc(size_t size, gfp_t gfp)
> > +{
> > +     struct qcom_scm_mem_chunk *chunk;
> > +     unsigned long vaddr;
>
> there are places below where you unnecessarily typecast this to its
> given type
>

Ah, it's a leftover from when this variable was of type void *. Thanks.

> > +     int ret;
> > +
> > +     if (!size)
> > +             return ZERO_SIZE_PTR;
> > +
> > +     size = roundup(size, 1 << PAGE_SHIFT);
> > +
> > +     chunk = kzalloc(sizeof(*chunk), gfp);
> > +     if (!chunk)
> > +             return NULL;
> > +
> > +     vaddr = gen_pool_alloc(qcom_scm_mem.pool, size);
> > +     if (!vaddr) {
> > +             kfree(chunk);
> > +             return NULL;
> > +     }
> > +
> > +     chunk->paddr = gen_pool_virt_to_phys(qcom_scm_mem.pool,
> > +                                          (unsigned long)vaddr);
>
> unnecessary typecast?
>
> > +     chunk->size = size;
> > +
> > +     scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock) {
>
> my first exposure to this infrastructure..very cool now that I've
> wrapped my head around it! This helped for those also new to this:
> https://lwn.net/Articles/934679/
>

It's amazing but be careful with it. I used it wrong in one place and
got yelled at by Linus Torvalds personally already. :)

Bartosz

> > +             ret = radix_tree_insert(&qcom_scm_mem.chunks, vaddr, chunk);
> > +             if (ret) {
> > +                     gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr,
>
> unnecessary typecast?
>
> > +                                   chunk->size);
> > +                     kfree(chunk);
> > +                     return NULL;
> > +             }
> > +     }
> > +
> > +     return (void *)vaddr;
> > +}
>