From owner-freebsd-net@freebsd.org Sat Dec 3 05:20:18 2016 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DE16FC6417A for ; Sat, 3 Dec 2016 05:20:18 +0000 (UTC) (envelope-from rysto32@gmail.com) Received: from mail-io0-x233.google.com (mail-io0-x233.google.com [IPv6:2607:f8b0:4001:c06::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AC2FB1A76 for ; Sat, 3 Dec 2016 05:20:18 +0000 (UTC) (envelope-from rysto32@gmail.com) Received: by mail-io0-x233.google.com with SMTP id a124so517185222ioe.2 for ; Fri, 02 Dec 2016 21:20:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yl0Eo3+al2XVFSMoRA2rxGseoA9xZ4AOUNz7gKV9faE=; b=CiwcsOBlBxsu5CA9VLbVmKr1HjhTISGxM56Az/9JomE3ETdHbzLmbee15WlCUvazUI ZGgn7uCh06g2QqvpAfQiJQH/wgAznfoxGaJ1h2GuMqPc42B7+k4zMSbGp/5bZ18LZNwI 7Ot+T0VXehS/QPZub3t/EXnNN1PTkmLYgdgxV/8LCvrniEctQ1K948qQIDh01vngA2qp 1OoOQCEBGamUTsz4gPPDQKgsgNmBDkBpEy2wYqJwMNliVBFr85Wk1C3HLBScwpC52EjX KgWAmmXB+0q0FshtZKMhNyRxvHiQfbYrutDfimwQy4AbTLbCVLH+SpHvsEuwFu/DNb2m w/Qw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yl0Eo3+al2XVFSMoRA2rxGseoA9xZ4AOUNz7gKV9faE=; b=Cd3KzN+IWS+XoaHL2U1n0tcovXerpIGVaWxCyifL4tpLPr6kUikWqoivBJHflj8LX6 XjnVpTixspEZEDrASuD5IZ2ZVEP3SAO/S2taTUvjw3zgmyTLwYZwPN5nrbxFzjRlut7t 8mbkBLW/GxnojbOVvj9m1/6wbwRzKcBuQRDtDeL4ynvSoDGFs66dG2jnP+10rKS1VUF0 fuAYr8P6OhK4twSbA2lTw3581FAWguIF8CLXswQ5AXFuHo4q7s0yC7GGWIK0tjEWmbSL Pf7BgZZLc2H3jTLzBsYaUsrlEYD3Ry0NJW5E6wD5T33pedSYt+4RaDJukVvOv1zK4mZv ekYQ== X-Gm-Message-State: AKaTC03BRkfK2Q69+uVQWksMs0RyWEaww8qzmcAYkt3Oz/rciJZD3Z0xAXmX3yXqeVcKw47/t3V18aaHIPc2tw== X-Received: by 10.107.136.164 with SMTP id s36mr41987235ioi.214.1480742418228; Fri, 02 Dec 2016 21:20:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.144.84 with HTTP; Fri, 2 Dec 2016 21:20:17 -0800 (PST) In-Reply-To: <201612022042.uB2KgOkO055419@elf.torek.net> References: <201612022042.uB2KgOkO055419@elf.torek.net> From: Ryan Stone Date: Sat, 3 Dec 2016 00:20:17 -0500 Message-ID: Subject: Re: mutex usage in if_bridge vs other drivers To: Chris Torek Cc: freebsd-net Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 03 Dec 2016 05:20:19 -0000 On Fri, Dec 2, 2016 at 3:42 PM, Chris Torek wrote: > THE QUESTION: > > - Who is wrong, the bxe driver or the bridge code? I.e., > does the bridge driver need to release its lock here, > and if so, is that actually safe to do? (We might need > to restart the loop over all the members if we drop the > lock.) > > - Or if the bridge driver should retain its lock, can it > use an sx lock here, to permit members to also use sx > locks? > bxe is not the only driver that sleeps in its config path, so I would say that if_bridge is de facto incorrect. Dropping the lock is entirely the wrong thing to do -- as you note, if we do, then the bridge members can change out from under us. The only path forward is to use an sx lock, but unfortunately it's not quite as simple as just converting the lock to an sx lock. The problem is that BRIDGE_LOCK() is also called in the transmit and receive paths, and those paths absolutely may not sleep for any reason. I believe that the correct fix would be to introduce an sx lock to if_bridge in additional to the current lock. In code paths that call ioctls() on bridge members, replace the use of BRIDGE_LOCK with the new sx lock. In code paths that modify the list of bridge members, hold both the BRIDGE_LOCK and the new sx lock. In the transmit and receive paths, nothing should change.