linux-kernel - Re: [PATCH] PCI/doe: Fix work struct declaration

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:   Tue, 15 Nov 2022 16:12:58 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Gregory Price <gregory.price@...verge.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>, linux-cxl@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH] PCI/doe: Fix work struct declaration

On Tue, Nov 15, 2022 at 12:54:39PM -0800, Ira Weiny wrote:
> On Tue, Nov 15, 2022 at 02:41:35PM -0600, Bjorn Helgaas wrote:
> > On Tue, Nov 15, 2022 at 12:18:38PM -0800, Ira Weiny wrote:
> > > On Tue, Nov 15, 2022 at 01:44:24PM -0600, Bjorn Helgaas wrote:
> > > > On Mon, Nov 14, 2022 at 05:19:43PM -0800, ira.weiny@...el.com wrote:
> > > > > From: Ira Weiny <ira.weiny@...el.com>
> > > > > 
> > > > > The callers of pci_doe_submit_task() allocate the
> > > > > pci_doe_task on the stack.  This causes the work structure
> > > > > to be allocated on the stack without pci_doe_submit_task()
> > > > > knowing.  Work item initialization needs to be done with
> > > > > either INIT_WORK_ONSTACK() or INIT_WORK() depending on how
> > > > > the work item is allocated.
> > > > > 
> > > > > Jonathan suggested creating doe task allocation macros such
> > > > > as DECLARE_CDAT_DOE_TASK_ONSTACK().[1]  The issue with this
> > > > > is the work function is not known to the callers and must be
> > > > > initialized correctly.
> > > > > 
> > > > > A follow up suggestion was to have an internal
> > > > > 'pci_doe_work' item allocated by pci_doe_submit_task().[2]
> > > > > This requires an allocation which could restrict the context
> > > > > where tasks are used.
> > > > > 
> > > > > Compromise with an intermediate step to initialize the task
> > > > > struct with a new call pci_doe_init_task() which must be
> > > > > called prior to submit task.
> > > > 
> > > > I'm not really a fan of passing a parameter to say "this struct is on
> > > > the stack" because that seems kind of error-prone and I don't know
> > > > what the consequence of getting it wrong would be.  Sounds like it
> > > > *could* be some memory corruption or reading garbage data that would
> > > > be hard to debug.
> > > > 
> > > > Do we have cases today where pci_doe_submit_task() can't do the
> > > > kzalloc() as in your patch at [3]?
> 
> No.
> 
> > > > If the current use cases allow a
> > > > kzalloc(), why not do that now and defer this until it becomes an
> > > > issue?
> 
> I do like pci_doe_submit_task() handling this as an internal detail.
> I'm happy with that if you are.
> 
> I was just concerned about the restriction of context.  Dan
> suggested this instead of passing a gfp parameter.
> 
> If you are happy with my original patch I will submit it instead.
> (With a better one liner.)

I don't know what's coming as far as pci_doe_submit_task() callers.
If there's some imminent caller that will require atomic context, I
guess we could solve it now.  But DOE doesn't really seem like an
atomic context thing to begin with, so maybe we could postpone dealing
with it.

That patch in [3] is more complicated than I expected, but I admit I
haven't looked closely.

Bjorn

> > > > > [1] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m88a7f50dcce52f30c8bf5c3dcc06fa9843b54a2d
> > > > > [2] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m63c636c5135f304480370924f4d03c00357be667
> > > > 
> > > > [3] https://lore.kernel.org/linux-cxl/Y2AnKB88ALYm9c5L@iweiny-desk3/