Re: [net-next v44] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Adam,

> > The LE condition makes sense, as the driver does not support BE. But
> > what is requiring 64-bit?
> The code itself will not, as written, work on a 32 Bit system, as there 
> are 64 bit specific code.

Yeah, that was more my question - what is 64-bit specific about the
code?

> > The comment is:
> > 
> >    Implementation of MCTP over PCC DMTF Specification DSP0256
> > 
> > But DSP0256 is not the "MCTP over PCC" transport binding spec. Either
> > mention the host interface specifically, or use the correct number for
> > the PCC spec. I think the latter would be best, as the host interface
> > spec seems less relevant here.
> 
> Yeah, just going to drop that line as it is redundant with other info in 
> the header.

OK.

> > > > > +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
> > > > > +{
> > > > > +       struct acpi_pcct_ext_pcc_shared_memory pcc_header;
> > > > > +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> > > > > +       struct mctp_pcc_mailbox *inbox;
> > > > > +       struct mctp_skb_cb *cb;
> > > > > +       struct sk_buff *skb;
> > > > > +       int size;
> > > > > +
> > > > > +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
> > > > > +       inbox = &mctp_pcc_ndev->inbox;
> > > > > +       memcpy_fromio(&pcc_header, inbox->chan->shmem, sizeof(pcc_header));
> > > > [High]
> > > > Can this dereference a NULL shmem during teardown?
> > > > 
> > > > Looking at pcc_mbox_free_channel() in drivers/mailbox/pcc.c, the
> > > > channel's shmem is iounmapped (and the pointer cleared) before the
> > > > shutdown path runs free_irq(). Until free_irq() returns, an inbound
> > > > doorbell on another CPU can still fire pcc_mbox_irq, which calls
> > > > mbox_chan_received_data(), which calls chan->cl->rx_callback (cl is
> > > > still set at that moment) — landing here.
> > > > 
> > > > Existing PCC clients such as drivers/acpi/acpi_pcc.c::pcc_rx_callback
> > > > do not touch shmem from their callback, so this driver appears to be
> > > > the first one to expose this ordering.
> > > > 
> > > > Should the rx_callback bail out when inbox->chan->shmem is NULL, or
> > > > should the ordering in pcc_mbox_free_channel() (free_irq before
> > > > iounmap) be addressed in the mailbox layer first?
> > > That needs to be fixed in the PCC layer, and is submitted in a different
> > > patch.  When that patch is accepted, this issue will be fixed.  I do not
> > > want to hold up this driver for that issue.
> > Understood that there's a fix in development for this, but we can't
> > introduce a potential null pointer dereference in the interim.
> > 
> > If you want this merged before the fix, how about a guard for a valid
> > shmem pointer?
> 
> The mutux used to control access to the create-teardown path is static 
> to drivers/mailbox/mailbox.c

You can't acquire that mutex here anyway, you're in atomic context.

> I could run mincore

No, that wouldn't work (and would be moderately horrific)

Since you did not answer the guard question, are you implying that the
shmem pointer may be unstable *during* the rx_complete call? (ie.,
rather than being invalidated before the call, and so a simple `if
(chan->shmem)` guard would not suffice?)

The Sashiko review seems to be incorrect in that you are not the first
user of the shmem data; there are other uses of chan->shmem that do not
seem to have any intrinsic exclusion between the rx callback and the
free_channel() paths, nor check the validity of shmem in their rx
callback.

However, the closest existing use - the xgene i2c driver - seems like it
can not free the pcc mailbox while a rx irq might occur, with the i2c
locking in place. You could do something similar here, possibly via a
spinlock acquired in both the ndo_stop handler, and the rx callback.

But if your proposed change to the iounmap() ordering works, that would
be a neater solution here.

> The patch that this gates on is:
> 
> https://lore.kernel.org/lkml/20260522205220.237355-1-admiyo@os.amperecomputing.com/
> 
> Please comment on that one to move it along.

As you can probably tell, the pcc mailbox interface is definitely not my
area of expertise.

Cheers,


Jeremy