From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752470Ab2ATLSh (ORCPT ); Fri, 20 Jan 2012 06:18:37 -0500 Received: from mail-bk0-f46.google.com ([209.85.214.46]:63409 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751546Ab2ATLSd convert rfc822-to-8bit (ORCPT ); Fri, 20 Jan 2012 06:18:33 -0500 MIME-Version: 1.0 In-Reply-To: <20120120100309.GA20640@debian> References: <20120116025331.GA16516@localhost> <1326991820-31393-1-git-send-email-rabin@rab.in> <20120120100309.GA20640@debian> Date: Fri, 20 Jan 2012 20:18:31 +0900 Message-ID: Subject: Re: [PATCHv2] backing-dev: fix wakeup timer races with bdi_unregister() From: Namjae Jeon To: Rabin Vincent Cc: fengguang.wu@intel.com, axboe@kernel.dk, linux-kernel@vger.kernel.org, chanho0207@gmail.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2012/1/20 Rabin Vincent : > On Fri, Jan 20, 2012 at 03:15:32PM +0900, Namjae Jeon wrote: >> 2012/1/20 Rabin Vincent : >> > On Fri, Jan 20, 2012 at 05:16, Namjae Jeon wrote: >> >>>                bdi_debug_unregister(bdi); >> >>> -               device_unregister(bdi->dev); >> >>> + >> >>> +               spin_lock_bh(&bdi->wb_lock); >> >>>                bdi->dev = NULL; >> >>> +               spin_unlock_bh(&bdi->wb_lock); >> >> Hi. >> >> Would you explain me why you add spinlock in here ? >> > >> > wakeup_timer_fn() does the following, where the >> > trace_writeback_wake_forker_thread() also accesses bdi->dev. >> > It does this under the wb_lock: >> > >> >        } else if (bdi->dev) { >> >                /* >> >                 * When bdi tasks are inactive for long time, they are killed. >> >                 * In this case we have to wake-up the forker thread which >> >                 * should create and run the bdi thread. >> >                 */ >> >                trace_writeback_wake_forker_thread(bdi); >> > >> > If we don't have the lock above, the bdi->dev could potentially be >> > cleared after the check but before the tracepoint is hit, leading to a >> > NULL pointer dereference. >> Is there no possibility trace_writeback_wake_forker_thread is called >> after spin_unlock of bdi->de= null ? > > wakeup_timer_fn() holds the wb_lock across the check for bdi->dev != > NULL and the call to trace_writeback_wake_forker_thread(), so no. The following case is not possible with racing of __mark_inode_dirty ? -------------------------------------------------------------------------------------------------- spin_lock bdi->dev = NULL; spin_unlock spin_lock ...... ...... trace_writeback_wake_forker_thread(bdi); spin_unlock -----------------------------------------------------------------------------