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

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

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 >>>
> >> > @@ -460,6 +461,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> >> > u_sysctl)
> >> >         ret = tmem_control(&op->u.tmem_op);
> >> >         break;
> >> > 
> >> > +    case XEN_SYSCTL_xsplice_op:
> >> > +        ret = xsplice_op(&op->u.xsplice);
> >> > +        copyback = (ret == -ENOSYS || ret == -EOPNOTSUPP) ? 0 : 1;
> >> 
> >> Why use a conditional expression here when its condition already is a 
> >> boolean one
> >> just needing negating?
> >
> >B/c I thought you would want it this way.
> >
> >I changed it to
> >
> >466         if ( ret != -ENOSYS && ret != -EOPNOTSUPP )
> >467             copyback = 1;
> 
> That's fine too.
> 
> >But I don't think this what you meant by 'negating'? As in:
> >
> >copyback = !rc ?
> >
> >But one of the subops returns the number of items and we definitly
> >want copyback=1 for that.
> 
> What I mean with "negating" was
> 
> copyback = (ret != -ENOSYS && ret != -EOPNOTSUPP);
> 
> >> > +    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?

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.

> possibly complicating the code.
> 
> Jan
> 

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