Bug 217401 - Deadlock in if_clone.c
Summary: Deadlock in if_clone.c
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.3-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: Andriy Voskoboinyk
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-02-27 16:51 UTC by Daan Vreeken
Modified: 2017-10-21 10:59 UTC (History)
4 users (show)

See Also:


Attachments
Fix for deadlock in if_clone.c (766 bytes, patch)
2017-02-27 19:50 UTC, Daan Vreeken
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daan Vreeken 2017-02-27 16:51:27 UTC
The code in if_clone.c uses the unit number allocator subsystem to keep track of free unit numbers when allocating interfaces. This can get out of sync with reality since interfaces can be renamed after creation. When this happens, the retry loop in if_alloc_unit() can deadlock a system.

The following set of commands will deadlock FreeBSD:
  ifconfig bridge create
  ifconfig bridge create
  ifconfig bridge1 name bridge3
  ifconfig bridge create
  ifconfig bridge create
  ifconfig bridge create
  # (deadlock at this point)

The deadlock happens when alloc_unr_specific() returns an unused unit number, but the later call to ifunit(name) sees that the unit is already in use.
In this case, the code will increase '*unit' and loop back to 'retry:'. If this increased '*unit' number is already allocated, the return value of the next call to alloc_unr_specific() will override '*unit' and leave it at -1.
The code will then increase '*unit' again and loop back to 'retry:' again with '*unit' now set to 0. If unit number 0 also already exists, we'll be caught in an infinite loop.

The attached patch fixes the deadlock by simply not destroying the value of '*unit' before incrementing it in the 'retry' path.
(This is a slight variant of kern/162789. The same use case that triggered that bug now also triggers this deadlock.)
Comment 1 Gleb Smirnoff freebsd_committer freebsd_triage 2017-02-27 19:22:12 UTC
Thanks for report, Daan. There is no attachment to the PR. I could reconstruct it from your description, but better you add it, so that we are on the same page.
Comment 2 Daan Vreeken 2017-02-27 19:50:30 UTC
Created attachment 180348 [details]
Fix for deadlock in if_clone.c

Here's the missing attachment.
The patch can also be downloaded from:
  http://www.vitsch.nl/pub_diffs/

Thanks!
Comment 3 commit-hook freebsd_committer freebsd_triage 2017-10-16 21:21:48 UTC
A commit references this bug:

Author: avos
Date: Mon Oct 16 21:21:31 UTC 2017
New revision: 324672
URL: https://svnweb.freebsd.org/changeset/base/324672

Log:
  ifnet(9): split ifc_alloc_unit() (should simplify code flow)

  Allocate smallest unit number from pool via ifc_alloc_unit_next()
  and exact unit number (if available) via ifc_alloc_unit_specific().

  While here, address possible deadlock (mentioned in PR).

  PR:		217401
  MFC after:	5 days
  Differential Revision:	https://reviews.freebsd.org/D12551

Changes:
  head/sys/net/if_clone.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2017-10-21 10:22:15 UTC
A commit references this bug:

Author: avos
Date: Sat Oct 21 10:21:34 UTC 2017
New revision: 324812
URL: https://svnweb.freebsd.org/changeset/base/324812

Log:
  MFC r324672:
  ifnet(9): split ifc_alloc_unit() (should simplify code flow)

  Allocate smallest unit number from pool via ifc_alloc_unit_next()
  and exact unit number (if available) via ifc_alloc_unit_specific().

  While here, address possible deadlock (mentioned in PR).

  PR:		217401
  Differential Revision:	https://reviews.freebsd.org/D12551

Changes:
_U  stable/11/
  stable/11/sys/net/if_clone.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2017-10-21 10:48:39 UTC
A commit references this bug:

Author: avos
Date: Sat Oct 21 10:48:06 UTC 2017
New revision: 324813
URL: https://svnweb.freebsd.org/changeset/base/324813

Log:
  MFC r324672:
  ifnet(9): split ifc_alloc_unit() (should simplify code flow)

  Allocate smallest unit number from pool via ifc_alloc_unit_next()
  and exact unit number (if available) via ifc_alloc_unit_specific().

  While here, address possible deadlock (mentioned in PR).

  PR:		217401
  Differential Revision:	https://reviews.freebsd.org/D12551

Changes:
_U  stable/10/
  stable/10/sys/net/if_clone.c
Comment 6 Andriy Voskoboinyk freebsd_committer freebsd_triage 2017-10-21 10:59:27 UTC
Should be fixed now.