Received-SPF: pass (google.com: domain of linux-kernel+bounces-82717-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 27 Feb 2024 06:20:06 +0100
From: Greg KH <gregkh@linuxfoundation.org>
To: RD Babiera <rdbabiera@google.com>
Cc: heikki.krogerus@linux.intel.com, badhri@google.com,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v2] usb: typec: altmodes/displayport: create sysfs nodes
 after assigning driver data
Message-ID: <2024022731-dusk-posture-b83f@gregkh>
References: <20240226234228.1768725-2-rdbabiera@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226234228.1768725-2-rdbabiera@google.com>

On Mon, Feb 26, 2024 at 11:42:29PM +0000, RD Babiera wrote:
> The DisplayPort driver's sysfs nodes may be present to the userspace before
> typec_altmode_set_drvdata() completes in dp_altmode_probe. This means that
> a sysfs read can trigger a NULL pointer error by deferencing dp->hpd in
> hpd_show or dp->lock in pin_assignment_show, as dev_get_drvdata() returns
> NULL in those cases.
> 
> Create sysfs nodes after typec_altmode_set_drvdata call.
> 
> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
> Changes from v1:
> * Moved sysfs node creation instead of NULL checking dev_get_drvdata().
> ---
>  drivers/usb/typec/altmodes/displayport.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 5a80776c7255..5bbdd2c04237 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -731,10 +731,6 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	      DP_CAP_PIN_ASSIGN_DFP_D(alt->vdo)))
>  		return -ENODEV;
>  
> -	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
> -	if (ret)
> -		return ret;
> -
>  	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
>  	if (!dp)
>  		return -ENOMEM;
> @@ -766,6 +762,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	if (plug)
>  		typec_altmode_set_drvdata(plug, dp);
>  
> +	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
> +	if (ret)
> +		return ret;
> +

Now I am going to push back again and ask why you are even attempting to
create sysfs files "by hand" here at all?

Why is this just not set to be a default group?  That way the group is
managed properly by the driver core and the driver doesn't have to worry
about ANY of this at all.  Bonus is that you remove the "you raced with
userspace and lost" problem that this code still has even with the
change you made here.

Huge hint, if you EVER have to call sysfs_*() from a driver, usually
something is wrong (there are a few exceptions, this is not one.)

thanks,

greg k-h