Re: [PATCH v2 4/4] platform/x86/amd/hsmp: Ensure initialization of the sock struct in hsmp_send_message()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]

On Sat, 2 Aug 2025, Suma Hegde wrote:
> On 7/24/2025 2:50 PM, Suma Hegde wrote:
> > On 7/23/2025 12:40 PM, Ilpo Järvinen wrote:
> > > On Wed, 23 Jul 2025, Suma Hegde wrote:
> > > 
> > > > If all sockets are not probed, invoking hsmp_send_message() might result
> > > > in
> > > > unexpected behavior due to accessing an uninitialized socket structure.
> > > > 
> > > > The initialization of the sock structure can be confirmed if sock->dev
> > > > is initialized.
> > > > 
> > > > Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> > > > Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> > > > ---
> > > > Changes since v1:
> > > > New patch to address the partial probe/removal issue.
> > > > 
> > > >   drivers/platform/x86/amd/hsmp/acpi.c | 4 ++++
> > > >   drivers/platform/x86/amd/hsmp/hsmp.c | 3 +++
> > > >   2 files changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/platform/x86/amd/hsmp/acpi.c
> > > > b/drivers/platform/x86/amd/hsmp/acpi.c
> > > > index 15c4cedc2759..a2d91d4a3e02 100644
> > > > --- a/drivers/platform/x86/amd/hsmp/acpi.c
> > > > +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> > > > @@ -617,6 +617,10 @@ static int hsmp_acpi_probe(struct platform_device
> > > > *pdev)
> > > > 
> > > >   static void hsmp_acpi_remove(struct platform_device *pdev)
> > > >   {
> > > > +     struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
> > > > +
> > > > +     sock->dev = NULL;
> > > > +
> > > >        guard(mutex)(&hsmp_lock);
> > > >        /*
> > > >         * We register only one misc_device even on multi-socket system.
> > > > diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > b/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > index e05d824045d6..a4420db42781 100644
> > > > --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> > > > @@ -219,6 +219,9 @@ int hsmp_send_message(struct hsmp_message *msg)
> > > >                return -ENODEV;
> > > >        sock = &hsmp_pdev.sock[msg->sock_ind];
> > > > 
> > > > +     if (!sock->dev)
> > > > +             return -ENODEV;
> > > > +
> > > >        ret = down_interruptible(&sock->hsmp_sem);
> > > >        if (ret < 0)
> > > >                return ret;
> > > This is still racy. AFAICT, nothing prevents assigning NULL into sock->dev
> > > because of remove while hsmp_send_message() is running and that can result
> > > in dereferencing NULL.
> > 
> > Could you please suggest on how to resolve this issue?

Maybe rwsem would help, remove takes it for write and accessor for read.
But I've not really looked deeply into the details (summer time and 
everything).

> I will relocate hsmp_fops to acpi.c and plat.c from hsmp.c. Consequently, the
> module reference
> 
> count for hsmp_acpi should be incremented when the device file is opened.
> 
> Does this solution address the issue adequately? Is it acceptable?

I'm not sure how that addresses the NULL assignment, but perhaps I'll just 
need to see the code to follow it or maybe you were trying to suggest a 
solution into some entirely different problem?

-- 
 i.