Re: [PATCH v4 08/10] tpm: Driver for supporting multiple emulated TPMs

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Fri, Mar 04, 2016 at 04:16:41PM -0500, Stefan Berger wrote:
>    > > +static ssize_t vtpm_fops_write(struct file *filp, const char __user
>    *buf,
>    > > +                size_t count, loff_t *off)
>    > > +{
>    > > +   struct vtpm_dev *vtpm_dev = filp->private_data;
>    > > +
>    > > +   if (count > sizeof(vtpm_dev->buffer) ||
>    > > +       !(vtpm_dev->state & STATE_WAIT_RESPONSE_FLAG))
>    > > +      return -EIO;
>    >
>    > This is not atomic in any sense. You should move this statement after
>    > taking buf_lock.
> 
>    set_bit and friends were all atomic. Now we need to put all of these into
>    the mutex... ?

Hmm... but you have other fields too in struct vtpm_dev so you anyway
need to use mutex for concurrent access to the file. In that statement
you are accessing two fields from that structure.

Lets say someone forks 100 processes that concurrently access this
same open file. How else you would keep the state of vtpm_dev coherent
than protecting with a mutex?

Maybe the problem is that you are only thinking a proper way to use
this infrastructure?

>       Stefan

/Jarkko

------------------------------------------------------------------------------