linux-kernel - Re: [PATCH v5 04/13] xen/pvcalls: implement socket command and handle events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.10.1710191821370.27209@sstabellini-ThinkPad-X260>
Date:   Thu, 19 Oct 2017 18:26:18 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>
cc:     Stefano Stabellini <sstabellini@...nel.org>,
        xen-devel@...ts.xen.org, linux-kernel@...r.kernel.org,
        jgross@...e.com, Stefano Stabellini <stefano@...reto.com>
Subject: Re: [PATCH v5 04/13] xen/pvcalls: implement socket command and handle
 events

On Tue, 17 Oct 2017, Boris Ostrovsky wrote:
> On 10/06/2017 08:30 PM, Stefano Stabellini wrote:
> > Send a PVCALLS_SOCKET command to the backend, use the masked
> > req_prod_pvt as req_id. This way, req_id is guaranteed to be between 0
> > and PVCALLS_NR_REQ_PER_RING. We already have a slot in the rsp array
> > ready for the response, and there cannot be two outstanding responses
> > with the same req_id.
> >
> > Wait for the response by waiting on the inflight_req waitqueue and
> > check for the req_id field in rsp[req_id]. Use atomic accesses and
> > barriers to read the field. Note that the barriers are simple smp
> > barriers (as opposed to virt barriers) because they are for internal
> > frontend synchronization, not frontend<->backend communication.
> >
> > Once a response is received, clear the corresponding rsp slot by setting
> > req_id to PVCALLS_INVALID_ID. Note that PVCALLS_INVALID_ID is invalid
> > only from the frontend point of view. It is not part of the PVCalls
> > protocol.
> >
> > pvcalls_front_event_handler is in charge of copying responses from the
> > ring to the appropriate rsp slot. It is done by copying the body of the
> > response first, then by copying req_id atomically. After the copies,
> > wake up anybody waiting on waitqueue.
> >
> > socket_lock protects accesses to the ring.
> >
> > Create a new struct sock_mapping and convert the pointer into an
> > uint64_t and use it as id for the new socket to pass to the backend. The
> > struct will be fully initialized later on connect or bind. In this patch
> > the struct sock_mapping is empty, the fields will be added by the next
> > patch.
> >
> > sock->sk->sk_send_head is not used for ip sockets: reuse the field to
> > store a pointer to the struct sock_mapping corresponding to the socket.
> > This way, we can easily get the struct sock_mapping from the struct
> > socket.
> >
> > Signed-off-by: Stefano Stabellini <stefano@...reto.com>
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@...cle.com>
> 
> with one question:
> 
> > +	/*
> > +	 * PVCalls only supports domain AF_INET,
> > +	 * type SOCK_STREAM and protocol 0 sockets for now.
> > +	 *
> > +	 * Check socket type here, AF_INET and protocol checks are done
> > +	 * by the caller.
> > +	 */
> > +	if (sock->type != SOCK_STREAM)
> > +		return -ENOTSUPP;
> > +
> 
> 
> Is this ENOTSUPP or EOPNOTSUPP? I didn't know the former even existed
> and include/linux/errno.h suggests that this is NFSv3-specific.

The PVCalls spec says that unimplemented commands return ENOTSUPP,
defined as -524. I guess that is why I used ENOTSUPP, but, actually,
this is the return value to the caller, which has nothing to do with the
PVCalls protocol return value. In fact, it could be something entirely
different.

In this case, I think you are correct, it is best to use EOPNOTSUPP.
I'll make the change and retain your Reviewed-by, if that's OK for you.