Re: [PATCH v5 1/4] usb: typec: ucsi: Update UCSI structure to have message in and message out fields

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

Thu, Dec 18, 2025 at 03:22:43AM +0200, Dmitry Baryshkov kirjoitti:
> On 18/12/2025 03:17, Katiyar, Pooja wrote:
> > Hello,
> > 
> > On Fri, Dec 12, 2025 at 06:58:06PM -0800, Dmitry Baryshkov wrote:
> > > On Thu, Oct 30, 2025 at 07:48:55AM -0700, Pooja Katiyar wrote:
> > > > Update UCSI structure by adding fields for incoming and outgoing
> > > > messages. Update .sync_control function and other related functions
> > > > to use these new fields within the UCSI structure, instead of handling
> > > > them as separate parameters.
> > > > 
> > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > Signed-off-by: Pooja Katiyar <pooja.katiyar@intel.com>
> > > > ---
> > > > Changelog v3:
> > > > - Added message fields to UCSI structure and updated sync_control handling.
> > > 
> > > Colleagues, after looking at this patch I have a question. What prevents
> > > message_{in,out}{,_size} to be modified concurrently? While we have PPM
> > > lock around command submission, size fields and buffers are modified /
> > > accessed outside of the lock. Granted all the notifications and async
> > > nature of the UCSI and USB-C protocols, what prevents two commands from
> > > being executed at the same time with one of the execution threads
> > > accessing the results returned by the other command?
> > > 
> > > In other words:
> > > 
> > > - thread A sets message_in_size, calls ucsi_send_command(CMD_A), which
> > >    takes PPM lock
> > > 
> > >     - meanwhile thread B writes another value to message_in_size and
> > >       calls ucsi_send_command(CMD_B), which now waits on PPM lock
> > > 
> > > - thread A finishes execution of the CMD_A, copies data (with the
> > >    wrong size!) to the message_in_buf, then it releases PPM lock.
> > > 
> > > - thread A gets preempted
> > > 
> > >      - thread B gets scheduled, it takes PPM lock, executes CMD_B, gets
> > >        data into the message_in_buf and finally it releases PPM lock
> > > 
> > > - finally at some later point thread A gets scheduled, it accesses
> > >    message_in_buf, reading data that has been overwritten by CMD_B
> > >    execution.
> > > 
> > > WDYT?
> > 
> > Thank you for identifying this race condition. You are correct about the
> > concurrent access issue with the message buffer fields.
> > 
> > Here are two potential approaches I see to resolve this:
> > 
> > Option 1: Separate mutex locks for message variables
> > Add a dedicated message_lock mutex to protect message_{in,out}{,_size}.
> > message_{in,out}{,_size} will only be modified within the message_lock
> > protection.
> > 
> > 	mutex_lock(&ucsi->message_lock);
> > 	ucsi->message_in_size = size;
> > 	ret = ucsi_send_command(ucsi, cmd);
> > 	memcpy(buffer, ucsi->message_in, size);
> > 	mutex_unlock(&ucsi->message_lock);
> > 
> > Option 2: Pass message variables as function arguments
> > Pass message buffers and sizes as parameters down to where ppm_lock is
> > acquired, ensuring protection throughout command execution.
> > 
> > 	int ucsi_send_command(ucsi, cmd, msg_in_buf, msg_in_size, msg_out_buf,
> > 			      msg_out_size) {
> >      		mutex_lock(&ucsi->ppm_lock);
> >      		ucsi->message_in_size = msg_in_size;
> >      		// execute command and copy results to msg_in_buf
> > 		memcpy(msg_in_buf, ucsi->message_in, msg_in_size);
> >      		mutex_unlock(&ucsi->ppm_lock);
> > 	}
> > 
> > I'm leaning toward Option 1 as it resolves the concern of passing too many args.
> > What are your thoughts on the suggested options or do you have alternative
> > suggestions?
> 
> I'm slightly biased towards  the second option (it doesn't required extra
> locks and it also is a bit more idiomatic), but I'm fine either way.
> 
> Looking forward to seeing the fixing patch!

I don't think you are fully solving the problem.

Pooja, you know my opinion. This whole approach of using separate
files is wrong. IMO we should only have a single file for the entire
UCSI mailbox, and the mailbox should be allocated searately for
everyone that opens the file. I don't think there is any way to
guarantee that the data in these separate files isn't for somebody
else.

thanks,

-- 
heikki