netdev - Re: [PATCH net-next 4/6] vsock: add vsock

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]

Date:   Fri, 22 Nov 2019 09:25:46 +0000
From:   Stefan Hajnoczi <stefanha@...hat.com>
To:     Stefano Garzarella <sgarzare@...hat.com>
Cc:     Stefan Hajnoczi <stefanha@...il.com>, netdev@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        Dexuan Cui <decui@...rosoft.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jorgen Hansen <jhansen@...are.com>
Subject: Re: [PATCH net-next 4/6] vsock: add vsock_loopback transport

On Thu, Nov 21, 2019 at 04:25:17PM +0100, Stefano Garzarella wrote:
> On Thu, Nov 21, 2019 at 10:59:48AM +0100, Stefano Garzarella wrote:
> > On Thu, Nov 21, 2019 at 09:34:58AM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Nov 19, 2019 at 12:01:19PM +0100, Stefano Garzarella wrote:
> > > > +static struct workqueue_struct *vsock_loopback_workqueue;
> > > > +static struct vsock_loopback *the_vsock_loopback;
> > > 
> > > the_vsock_loopback could be a static global variable (not a pointer) and
> > > vsock_loopback_workqueue could also be included in the struct.
> > > 
> > > The RCU pointer is really a way to synchronize vsock_loopback_send_pkt()
> > > and vsock_loopback_cancel_pkt() with module exit.  There is no other
> > > reason for using a pointer.
> > > 
> > > It's cleaner to implement the synchronization once in af_vsock.c (or
> > > virtio_transport_common.c) instead of making each transport do it.
> > > Maybe try_module_get() and related APIs provide the necessary semantics
> > > so that core vsock code can hold the transport module while it's being
> > > used to send/cancel a packet.
> > 
> > Right, the module cannot be unloaded until open sockets, so here the
> > synchronization is not needed.
> > 
> > The synchronization come from virtio-vsock device that can be
> > hot-unplugged while sockets are still open, but that can't happen here.
> > 
> > I will remove the pointers and RCU in the v2.
> > 
> > Can I keep your R-b or do you prefer to watch v2 first?

I'd like to review v2.

> > > > +MODULE_ALIAS_NETPROTO(PF_VSOCK);
> > > 
> > > Why does this module define the alias for PF_VSOCK?  Doesn't another
> > > module already define this alias?
> > 
> > It is a way to load this module when PF_VSOCK is starting to be used.
> > MODULE_ALIAS_NETPROTO(PF_VSOCK) is already defined in vmci_transport
> > and hyperv_transport. IIUC it is used for the same reason.
> > 
> > In virtio_transport we don't need it because it will be loaded when
> > the PCI device is discovered.
> > 
> > Do you think there's a better way?
> > Should I include the vsock_loopback transport directly in af_vsock
> > without creating a new module?
> > 
> 
> That last thing I said may not be possible:
> I remembered that I tried, but DEPMOD found a cyclic dependency because
> vsock_transport use virtio_transport_common that use vsock, so if I
> include vsock_transport in the vsock module, DEPMOD is not happy.
> 
> Do you think it's okay in this case to keep MODULE_ALIAS_NETPROTO(PF_VSOCK)
> or is there a better way?

The reason I asked is because the semantics of duplicate module aliases
aren't clear to me.  Do all modules with the same alias get loaded?
Or just the first?  Or ...?

Stefan

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)