linux-kernel - Re: [PATCH v3 5/5] arm64: mte: add compression support to mteswap.c

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: <CAG_fn=Xvb9_K+vTLERVnNOSYMmn9+vt7SOMe_TJTAc=B0UtRKQ@mail.gmail.com>
Date:   Tue, 18 Jul 2023 12:48:00 +0200
From:   Alexander Potapenko <glider@...gle.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     catalin.marinas@....com, will@...nel.org, pcc@...gle.com,
        andreyknvl@...il.com, linux@...musvillemoes.dk,
        yury.norov@...il.com, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, eugenis@...gle.com,
        syednwaris@...il.com, william.gray@...aro.org
Subject: Re: [PATCH v3 5/5] arm64: mte: add compression support to mteswap.c

> > +#include <linux/mm_types.h>
>
> But you actually don't use that.
>
> struct page;
>
> forward declaration is enough.
Fair enough.
>
> > +void *_mte_alloc_and_save_tags(struct page *page);
> > +void _mte_free_saved_tags(void *tags);
> > +void _mte_restore_tags(void *tags, struct page *page);
> > +
> > +#endif // ARCH_ARM64_MM_MTESWAP_H_
>
> ...
>
> > +void _mte_free_saved_tags(void *storage)
> > +{
> > +     unsigned long handle = xa_to_value(storage);
> > +     int size;
> > +
> > +     if (!handle)
> > +             return;
>
> Perhaps
>
>         unsigned long handle;
>
>         handle = xa_to_value(storage);
>         if (!handle)
>                 return;

I don't have a strong preference and am happy to change this, but, out
of curiosity, why do you think it is better?
This pattern (calling (even non-)trivial functions when declaring
variables) is widely used across the kernel.
Or is it just for consistency with how `handle` is used in the rest of the file?


> > +void _mte_restore_tags(void *tags, struct page *page)
> > +{
>
> As per above.

Ack

> > +     if (try_page_mte_tagging(page)) {
> > +             if (!ea0_decompress(handle, tags_decomp))
> > +                     return;
> > +             mte_restore_page_tags(page_address(page), tags_decomp);
> > +             set_page_mte_tagged(page);
> > +     }
>
> I think you may drop an indentation level by
>
>         if (!try_page_mte_tagging(page))
>                 return;
>
> > +}

Ack

> ...
>
> > +void _mte_restore_tags(void *tags, struct page *page)
> > +{
> > +     if (try_page_mte_tagging(page)) {
> > +             mte_restore_page_tags(page_address(page), tags);
> > +             set_page_mte_tagged(page);
> > +     }
>
> Ditto.
Thanks!