Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Cong Wang <xiyou.wangcong@gmail.com>
To: Sean Tranchetti <stranche@codeaurora.org>
Cc: David Miller <davem@davemloft.net>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	Johannes Berg <johannes.berg@intel.com>,
	Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Subject: Re: [PATCH net v2] genetlink: remove genl_bind
Date: Tue, 30 Jun 2020 11:57:40 -0700
Message-ID: <CAM_iQpV9cL9Ems7pedZdg4Yai7iPFTNy78zKFQ96OM1JeLTUqA@mail.gmail.com> (raw)
In-Reply-To: <1593539417-19973-1-git-send-email-stranche@codeaurora.org>

On Tue, Jun 30, 2020 at 10:50 AM Sean Tranchetti
<stranche@codeaurora.org> wrote:
>
> A potential deadlock can occur during registering or unregistering a
> new generic netlink family between the main nl_table_lock and the
> cb_lock where each thread wants the lock held by the other, as
> demonstrated below.
>
> 1) Thread 1 is performing a netlink_bind() operation on a socket. As part
>    of this call, it will call netlink_lock_table(), incrementing the
>    nl_table_users count to 1.
> 2) Thread 2 is registering (or unregistering) a genl_family via the
>    genl_(un)register_family() API. The cb_lock semaphore will be taken for
>    writing.
> 3) Thread 1 will call genl_bind() as part of the bind operation to handle
>    subscribing to GENL multicast groups at the request of the user. It will
>    attempt to take the cb_lock semaphore for reading, but it will fail and
>    be scheduled away, waiting for Thread 2 to finish the write.
> 4) Thread 2 will call netlink_table_grab() during the (un)registration
>    call. However, as Thread 1 has incremented nl_table_users, it will not
>    be able to proceed, and both threads will be stuck waiting for the
>    other.
>
> genl_bind() is a noop, unless a genl_family implements the mcast_bind()
> function to handle setting up family-specific multicast operations. Since
> no one in-tree uses this functionality as Cong pointed out, simply removing
> the genl_bind() function will remove the possibility for deadlock, as there
> is no attempt by Thread 1 above to take the cb_lock semaphore.

I think it is worth noting removing the -ENOENT is probably okay,
as mentioned in commit 023e2cfa36c31b0ad28c159a1bb0d61ff57334c8:

    Also do this in generic netlink, and there also make sure that any
    bind for multicast groups that only exist in init_net is rejected.
    This isn't really a problem if it is accepted since a client in a
    different namespace will never receive any notifications from such
    a group, but it can confuse the family if not rejected (it's also
    possible to silently (without telling the family) accept it, but it
    would also have to be ignored on unbind so families that take any
    kind of action on bind/unbind won't do unnecessary work for invalid
    clients like that.

Thanks.

  reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 17:50 Sean Tranchetti
2020-06-30 18:57 ` Cong Wang [this message]
2020-07-01 22:49 ` David Miller

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=CAM_iQpV9cL9Ems7pedZdg4Yai7iPFTNy78zKFQ96OM1JeLTUqA@mail.gmail.com \
    --to=xiyou.wangcong@gmail.com \
    --cc=davem@davemloft.net \
    --cc=johannes.berg@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=stranche@codeaurora.org \
    --cc=subashab@codeaurora.org \
    /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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git