Re: [Xen-devel] [PATCH v8.1 04/27] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

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

On Mon, Apr 18, 2016 at 10:33:46AM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/18/16 9:50 AM >>>
> >On Sun, Apr 17, 2016 at 02:05:10AM -0600, Jan Beulich wrote:
> >> >>> Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> 04/15/16 4:29 AM >>>
> >> >On Thu, Apr 14, 2016 at 10:36:46AM -0600, Jan Beulich wrote:
> >> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:05 AM >>>
> >> >> > +    spin_lock(&payload_lock);
> >> >> > +
> >> >> > +    found = find_payload(n);
> >> >> > +    if ( IS_ERR(found) )
> >> >> > +    {
> >> >> > +        rc = PTR_ERR(found);
> >> >> > +        goto out;
> >> >> > +    }
> >> >> > +    else if ( found )
> >> >> > +    {
> >> >> > +        rc = -EEXIST;
> >> >> > +        goto out;
> >> >> > +    }
> >> >> > +
> >> >> > +    data = xzalloc(struct payload);
> >> >> 
> >> >> I generally advocate for not doing allocations with locks held, and I 
> >> >> don't think
> >> >> it would severely complicate the code here doing so.
> >> >
> >> >I can certainly unlock and then lock again (when adding
> >> >it to the list).
> >> 
> >> That would create a race again afaict. Instead what I have been trying to 
> >> hint
> >> at is that the allocation should be done before taking the lock, freeing 
> >> the object
> >> again if in the end it turned out it's not going to be needed. Hence the 
> >> referral to
> >
> >What if I get -ENOMEM and that the user supplied an payload we already
> >have? In that case I would return -ENOMEM while I would expect us to
> >return -EEXIST.
> >
> >Unless I add some extra checks to continue on?
> 
> Or unless you didn't check the allocation result right after the allocation 
> call,
> but only where you check it now.

Which is OK for data = xzalloc(struct payload);
That is not much data.

The issue is later (the 'Implement payload loading') is the allocation
of the raw_data which wants memory of size by ELF file.

This can be say 100kB or such.

> 
> >Also one could do a bit of memory DoS (perhaps by mistake) by continously
> >uploading the same and same payload and us first allocating the memory,
> >and then doing the check for the payload existence (which would then
> >free the memory). Since the allocation is outside the lock we can
> >eat a bit of memory.
> 
> Why that? You'd free the memory right away on the error path.
> 
> Jan
> 

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