Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933070AbdHVOVq (ORCPT ); Tue, 22 Aug 2017 10:21:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:43884 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933022AbdHVOVp (ORCPT ); Tue, 22 Aug 2017 10:21:45 -0400 Message-ID: <1503411523.6831.6.camel@suse.com> Subject: Re: Possible bug in cypress_m8.ko From: Oliver Neukum To: Anton Volkov , koyama@firstlight.net, dignome@gmail.com, johan@kernel.org Cc: Alexey Khoroshilov , gregkh@linuxfoundation.org, ldv-project@linuxtesting.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Date: Tue, 22 Aug 2017 16:18:43 +0200 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2342 Lines: 55 Am Dienstag, den 22.08.2017, 15:11 +0300 schrieb Anton Volkov: > Hello. > > Judging by the code of cypress_m8.c some functions are considered to be > capable of working concurrently with other functions, e.g. cypress_open. > There are, however, entities that are protected by the locks at one > place and not protected in another. Lines are given using the info from > Linux kernel v4.12. Example: > > cypress_send > spin_lock_irqsave > priv->write_urb_in_use = 1; > spin_lock_irqrestore > (cypress_m8.c: lines 761-763) > ... > if (result) { > priv->write_urb_in_use = 0; //without lock protection > (cypress_m8.c: line 783) > } > > Is it a bug? Yes, but not of the kind you describe. The transition from "not in use" to "in use" must be guarded by a lock, because it may be contended. But if that transition is properly guarded, you already know that there can be only user. He can theoretically give up the resource without locking. Yet there is a bug: ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 702) spin_lock_irqsave(&priv->lock, flags); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 703) if (priv->write_urb_in_use) { 441b62c1edb98 (Harvey Harrison 2008-03-03 16:08:34 -0800 704) dbg("%s - can't write, urb in use", __func__); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 705) spin_unlock_irqrestore(&priv->lock, flags); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 706) return; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 707) } ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 708) spin_unlock_irqrestore(&priv->lock, flags); The flag is checked is checked under a lock. But then the lock is dropped. And here: ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 759) spin_lock_irqsave(&priv->lock, flags); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 760) priv->write_urb_in_use = 1; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 761) spin_unlock_irqrestore(&priv->lock, flags); The flag is set under lock, but unconditionally. The code just makes no sense. In addition, when you drop the flag without a lock you need to worry about memory ordering. HTH Oliver