linux-kernel - Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

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: <20250905-speedy-giga-puma-1fede6@sudeepholla>
Date: Fri, 5 Sep 2025 15:37:21 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Adam Young <admiyo@...eremail.onmicrosoft.com>
Cc: <admiyo@...amperecomputing.com>, Jassi Brar <jassisinghbrar@...il.com>,
	Sudeep Holla <sudeep.holla@....com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Len Brown <lenb@...nel.org>, Robert Moore <robert.moore@...el.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Jeremy Kerr <jk@...econstruct.com.au>,
	Matt Johnston <matt@...econstruct.com.au>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the
 shared buffer

On Thu, Sep 04, 2025 at 01:06:09PM -0400, Adam Young wrote:
> Answers inline.
> 
> On 9/4/25 07:00, Sudeep Holla wrote:

[...]

> > Who will change this value as it is fixed to false always.
> > That makes the whole pcc_write_to_buffer() reduntant. It must go away.
> > Also why can't you use tx_prepare callback here. I don't like these changes
> > at all as I find these redundant. Sorry for not reviewing it in time.
> > I was totally confused with your versioning and didn't spot the mailbox/pcc
> > changes in between and assumed it is just MCTP net driver changes. My mistake.
> 
> This was a case of leaving the default as is to not-break the existing
> mailbox clients.
> 
> The maibox client can over ride it in its driver setup.
> 

What if driver changes in the middle of an ongoing transaction ? That
doesn't sound like a good idea to me.

You didn't respond as why tx_prepare callback can be used to do exactly
same thing ?

> > > +	void *(*rx_alloc)(struct mbox_client *cl,  int size);
> > Why this can't be in rx_callback ?
> 
> Because that is too late.
> 
> The problem is that the client needs  to allocate the memory that the
> message comes in in order to hand it off.
> 
> In the case of a network device, the rx_alloc code is going to return the
> memory are of a struct sk_buff. The Mailbox does not know how to allocate
> this. If the driver just kmallocs memory for the return message, we would
> have a re-copy of the message.
>

I still don't understand the requirement. The PCC users has access to shmem
and can do what they want in rx_callback, so I don't see any reason for
this API.

> This is really a mailbox-api level issue, but I was trying to limit the
> scope of my changes as much as possible.
> 

Please explain the issue. Sorry if I have missed, pointer are enough if
already present in some mail thread.

> The PCC mailbox code really does not match the abstractions of the mailbox
> in general.  The idea that copying into and out of the buffer is done by
> each individual driver leads to a lot of duplicated code.  With this change,
> most of the other drivers could now be re-written to let the mailbox manage
> the copying, while letting the mailbox client specify only how to allocate
> the message buffers.
> 

Yes that's because each user have their own requirement. You can do what
you want in rx_callback.

> Much of this change  was driven by the fact that the PCC mailbox does not
> properly check the flags before allowing writes to the rx channel, and that
> code is not exposed to the driver.  Thus, it was impossible to write
> everything in the rx callback regardless. This work was based on Huisong's
> comments on version 21 of the patch series.
> 

Pointers please, sorry again. But I really don't like the merged code and
looking for ways to clean it up as well as address the requirement if it
is not available esp. if we have to revert this change.

-- 
Regards,
Sudeep