linux-kernel - Re: [PATCH v2 3/4] cachestat: implement cachestat syscall

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:   Mon, 12 Dec 2022 08:37:39 -0800
From:   Nhat Pham <nphamcs@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Al Viro <viro@...iv.linux.org.uk>, akpm@...ux-foundation.org,
        hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, bfoster@...hat.com,
        kernel-team@...a.com
Subject: Re: [PATCH v2 3/4] cachestat: implement cachestat syscall

On Mon, Dec 12, 2022 at 8:24 AM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Mon, Dec 12, 2022 at 08:23:31AM -0800, Nhat Pham wrote:
> > > It would be easier to read if you inverted the condition here.
> >
> > Oh I think I tried
> >
> > if (!f.file)
> >        return -EBADF;
> >
> > here, but there are some mixing-code-with-decl warnings.
> > If I recall correctly, the problem is with this line:
> >
> > XA_STATE(xas, &mapping->i_pages, first_index);
> >
> > which is expanded into a declaration:
> >
> > #define XA_STATE(name, array, index) \
> > struct xa_state name = __XA_STATE(array, index, 0, 0)
> >
> > It requires a valid mapping though, which is
> > obtained from f.file:
> >
> > struct address_space *mapping = f.file->f_mapping;
> >
> > so it cannot be moved above the if(!f.file) check either...
>
> Perhaps you're trying to do too much in a single function?

That's fair. In an ancient version of this, I did try to separate
the folio-walking logic into its own function, but then decided
against it because it seemed unnecessary. That was before
the complexity of that piece of logic blew up to the current
situation though, so perhaps it is time to revisit this decision
and refactor it.