LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Miller <davem@davemloft.net>
To: sgarzare@redhat.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
	stefanha@redhat.com, mst@redhat.com
Subject: Re: [PATCH 1/4] vsock/virtio: fix locking around 'the_virtio_vsock'
Date: Wed, 29 May 2019 21:28:52 -0700 (PDT)	[thread overview]
Message-ID: <20190529.212852.1077585415381753122.davem@davemloft.net> (raw)
In-Reply-To: <20190528105623.27983-2-sgarzare@redhat.com>

From: Stefano Garzarella <sgarzare@redhat.com>
Date: Tue, 28 May 2019 12:56:20 +0200

> @@ -68,7 +68,13 @@ struct virtio_vsock {
>  
>  static struct virtio_vsock *virtio_vsock_get(void)
>  {
> -	return the_virtio_vsock;
> +	struct virtio_vsock *vsock;
> +
> +	mutex_lock(&the_virtio_vsock_mutex);
> +	vsock = the_virtio_vsock;
> +	mutex_unlock(&the_virtio_vsock_mutex);
> +
> +	return vsock;

This doesn't do anything as far as I can tell.

No matter what, you will either get the value before it's changed or
after it's changed.

Since you should never publish the pointer by assigning it until the
object is fully initialized, this can never be a problem even without
the mutex being there.

Even if you sampled the the_virtio_sock value right before it's being
set to NULL by the remove function, that still can happen with the
mutex held too.

This function is also terribly named btw, it implies that a reference
count is being taken.  But that's not what this function does, it
just returns the pointer value as-is.

  reply	other threads:[~2019-05-30  4:29 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-28 10:56 [PATCH 0/4] vsock/virtio: several fixes in the .probe() and .remove() Stefano Garzarella
2019-05-28 10:56 ` [PATCH 1/4] vsock/virtio: fix locking around 'the_virtio_vsock' Stefano Garzarella
2019-05-30  4:28   ` David Miller [this message]
2019-05-30 10:27     ` Stefano Garzarella
2019-05-28 10:56 ` [PATCH 2/4] vsock/virtio: stop workers during the .remove() Stefano Garzarella
2019-05-28 10:56 ` [PATCH 3/4] vsock/virtio: fix flush of works " Stefano Garzarella
2019-05-29  3:22   ` Jason Wang
2019-05-29 10:58     ` Stefano Garzarella
2019-05-30  9:46       ` Jason Wang
2019-05-30 10:10         ` Stefano Garzarella
2019-05-30 11:59           ` Jason Wang
2019-05-31  8:18             ` Stefano Garzarella
2019-05-31  9:56               ` Jason Wang
2019-06-06  8:11                 ` Stefano Garzarella
2019-06-13  8:57                   ` Jason Wang
2019-06-27 10:26                     ` Stefano Garzarella
2019-05-28 10:56 ` [PATCH 4/4] vsock/virtio: free used buffers " Stefano Garzarella
2019-06-10 13:09 ` [PATCH 0/4] vsock/virtio: several fixes in the .probe() and .remove() Stefan Hajnoczi
2019-06-27 10:05   ` Stefano Garzarella

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190529.212852.1077585415381753122.davem@davemloft.net \
    --to=davem@davemloft.net \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=sgarzare@redhat.com \
    --cc=stefanha@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    --subject='Re: [PATCH 1/4] vsock/virtio: fix locking around '\''the_virtio_vsock'\''' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).