This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


[Xen-devel] Broken (and unnecessary) SMP locking in blktap.c

To: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] Broken (and unnecessary) SMP locking in blktap.c
From: "Stephen C. Tweedie" <sct@xxxxxxxxxx>
Date: Wed, 27 Sep 2006 14:41:18 +0100
Delivery-date: Wed, 27 Sep 2006 06:41:59 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Hi all,

The kernel blktap.c has totally broken locking in req_increase().  It
takes an IRQ lock, then calls a number of non-atomic MM functions, such
as balloon_alloc_empty_page_range() and kzalloc(GFP_KERNEL).  So built
with SMP debugging, this function BUG()s instantly on driver

The locking is actually completely unnecessary too in its current form,
because req_increase() is only ever called right at the start of driver

static int __init blkif_init(void)
        int i,ret,blktap_dir;
        tap_blkif_t *info;

        if (!is_running_on_xen())
                return -ENODEV;

        for(i = 0; i < 2; i++) {
                ret = req_increase();
                if (ret)

is the only caller.  So nothing else in the driver can possibly be
active at this time.

The *only* reason it might be needed is to provide locking for some
hypothetical future use of req_increase() and req_decrease(), which
would need to be locked against each other.  Given that this isn't done
right now, I suggest we nuke both the locking and the (unused)
req_decrease function; that can easily be resurrected with correct
locking if we want it in the future.

Anyone mind if that function dies?


Xen-devel mailing list

<Prev in Thread] Current Thread [Next in Thread>