Re: [Xen-devel] [PATCH v5 26/28] xsplice: Prevent duplicate payloads from being loaded.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Apr 04, 2016 at 09:06:47AM -0600, Jan Beulich wrote:
> >>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> > --- a/xen/common/xsplice.c
> > +++ b/xen/common/xsplice.c
> > @@ -566,6 +566,27 @@ static int prepare_payload(struct payload *payload,
> >          if ( !payload->id.len || !payload->id.p )
> >              return -EINVAL;
> >      }
> > +    /* Make sure it is not a duplicate. */
> > +    if ( payload->id.len )
> 
> The conditional is pointless considering the one visible in context
> above.

/me nods. Let me move it inside the braces above.
> 
> > +    {
> > +        struct payload *data;
> > +
> > +        spin_lock_recursive(&payload_lock);
> > +        list_for_each_entry ( data, &payload_list, list )
> > +        {
> > +            /* No way payload is on the list. */
> > +            ASSERT( data != payload );
> > +            if ( data->id.len &&
> > +                 !memcmp(data->id.p, payload->id.p, data->id.len) )
> > +            {
> > +                spin_unlock_recursive(&payload_lock);
> > +                dprintk(XENLOG_DEBUG, "%s%s: Already loaded as %s!\n",
> > +                        XSPLICE, elf->name, data->name);
> > +                return -EEXIST;
> > +            }
> > +        }
> > +        spin_unlock_recursive(&payload_lock);
> 
> Similar question as asked elsewhere - with the lock getting dropped
> here, how is the "no duplicate" state going to be ensured by the
> time you actually load and insert this payload?

It won't. I've removed the spinlock usage here and we are keeping the spinlock
held at xsplice_upload.
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel