Patch Detail

HTTP 200 OK
Allow: GET, PUT, PATCH, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "id": 870,
    "url": "http://patchwork.ozlabs.org/api/patches/870/?format=api",
    "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/20080922114256.GA27055@gondor.apana.org.au/",
    "project": {
        "id": 7,
        "url": "http://patchwork.ozlabs.org/api/projects/7/?format=api",
        "name": "Linux network development",
        "link_name": "netdev",
        "list_id": "netdev.vger.kernel.org",
        "list_email": "netdev@vger.kernel.org",
        "web_url": null,
        "scm_url": null,
        "webscm_url": null,
        "list_archive_url": "",
        "list_archive_url_format": "",
        "commit_url_format": ""
    },
    "msgid": "<20080922114256.GA27055@gondor.apana.org.au>",
    "list_archive_url": null,
    "date": "2008-09-22T11:42:56",
    "name": "xfrm_state locking regression...",
    "commit_ref": null,
    "pull_url": null,
    "state": "accepted",
    "archived": true,
    "hash": "589277ea57ae65985fb8141c15b697cfde5ef035",
    "submitter": {
        "id": 357,
        "url": "http://patchwork.ozlabs.org/api/people/357/?format=api",
        "name": "Herbert Xu",
        "email": "herbert@gondor.apana.org.au"
    },
    "delegate": {
        "id": 34,
        "url": "http://patchwork.ozlabs.org/api/users/34/?format=api",
        "username": "davem",
        "first_name": "David",
        "last_name": "Miller",
        "email": "davem@davemloft.net"
    },
    "mbox": "http://patchwork.ozlabs.org/project/netdev/patch/20080922114256.GA27055@gondor.apana.org.au/mbox/",
    "series": [],
    "comments": "http://patchwork.ozlabs.org/api/patches/870/comments/",
    "check": "pending",
    "checks": "http://patchwork.ozlabs.org/api/patches/870/checks/",
    "tags": {},
    "related": [],
    "headers": {
        "Return-Path": "<netdev-owner@vger.kernel.org>",
        "X-Original-To": "patchwork-incoming@ozlabs.org",
        "Delivered-To": "patchwork-incoming@ozlabs.org",
        "Received": [
            "from vger.kernel.org (vger.kernel.org [209.132.176.167])\n\tby ozlabs.org (Postfix) with ESMTP id 395DCDDE00\n\tfor <patchwork-incoming@ozlabs.org>;\n\tMon, 22 Sep 2008 21:43:19 +1000 (EST)",
            "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751857AbYIVLnI (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tMon, 22 Sep 2008 07:43:08 -0400",
            "(majordomo@vger.kernel.org) by vger.kernel.org id S1751749AbYIVLnH\n\t(ORCPT <rfc822; netdev-outgoing>); Mon, 22 Sep 2008 07:43:07 -0400",
            "from rhun.apana.org.au ([64.62.148.172]:36726 \"EHLO\n\tarnor.apana.org.au\" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org\n\twith ESMTP id S1751645AbYIVLnG (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Mon, 22 Sep 2008 07:43:06 -0400",
            "from gondolin.me.apana.org.au ([192.168.0.6] ident=Debian-exim)\n\tby arnor.apana.org.au with esmtp (Exim 4.63 #1 (Debian))\n\tid 1Khjog-00054h-8H; Mon, 22 Sep 2008 21:42:58 +1000",
            "from herbert by gondolin.me.apana.org.au with local (Exim 4.69)\n\t(envelope-from <herbert@gondor.apana.org.au>)\n\tid 1Khjof-0000z9-5d; Mon, 22 Sep 2008 19:42:57 +0800"
        ],
        "Date": "Mon, 22 Sep 2008 19:42:56 +0800",
        "From": "Herbert Xu <herbert@gondor.apana.org.au>",
        "To": "Timo =?iso-8859-1?Q?Ter=E4s?= <timo.teras@iki.fi>",
        "Cc": "David Miller <davem@davemloft.net>, netdev@vger.kernel.org",
        "Subject": "Re: xfrm_state locking regression...",
        "Message-ID": "<20080922114256.GA27055@gondor.apana.org.au>",
        "References": "<48BE329C.2010209@iki.fi>\n\t<20080902.234723.163403187.davem@davemloft.net>\n\t<20080905115506.GA26179@gondor.apana.org.au>\n\t<20080908.172513.162820960.davem@davemloft.net>\n\t<20080909143312.GA29952@gondor.apana.org.au>\n\t<48D63E3A.90301@iki.fi> <48D66677.2040309@iki.fi>",
        "MIME-Version": "1.0",
        "Content-Type": "text/plain; charset=iso-8859-1",
        "Content-Disposition": "inline",
        "Content-Transfer-Encoding": "8bit",
        "In-Reply-To": "<48D66677.2040309@iki.fi>",
        "User-Agent": "Mutt/1.5.18 (2008-05-17)",
        "Sender": "netdev-owner@vger.kernel.org",
        "Precedence": "bulk",
        "List-ID": "<netdev.vger.kernel.org>",
        "X-Mailing-List": "netdev@vger.kernel.org"
    },
    "content": "On Sun, Sep 21, 2008 at 06:21:27PM +0300, Timo Teräs wrote:\n>\n> But the flaw still exists: the entries which interator->next points\n> can be deleted if there is a walking that takes a long time and\n> meanwhile we get walks that complete fast.\n\nThanks, I guess that's why I'm not paid to work on RCU :)\n\n> So if we do list_del_rcu() on delete, could we also xfrm_state_hold()\n> the entry pointed to by that list entry. And then on GC we could\n> xfrm_state_put() the next entry.\n\nUnfortunately it's not that simple since we'll be in the same\nbind if the entry after the next entry gets deleted as well as\nthe next entry.\n\nThe only other solution is to go back to the original scheme\nwhere we keep the list intact until GC.  However, nobody has\ncome up with a way of doing that that allows the SA creation\npath to run locklessly with respect to the dumping path.\n\nSo I think we'll have to stick with the quasi-RCU solution for\nnow.  Here's a patch to fix the flaw that you discovered.\n\nipsec: Fix xfrm_state_walk race\n\nAs discovered by Timo Teräs, the currently xfrm_state_walk scheme\nis racy because if a second dump finishes before the first, we\nmay free xfrm states that the first dump would walk over later.\n\nThis patch fixes this by storing the dumps in a list in order\nto calculate the correct completion counter which cures this\nproblem.\n\nI've expanded netlink_cb in order to accomodate the extra state\nrelated to this.  It shouldn't be a big deal since netlink_cb\nis kmalloced for each dump and we're just increasing it by 4 or\n8 bytes.\n\nSigned-off-by: Herbert Xu <herbert@gondor.apana.org.au>\n\n\nCheers,",
    "diff": "diff --git a/include/linux/netlink.h b/include/linux/netlink.h\nindex 9ff1b54..cbba776 100644\n--- a/include/linux/netlink.h\n+++ b/include/linux/netlink.h\n@@ -220,7 +220,7 @@ struct netlink_callback\n \tint\t\t(*dump)(struct sk_buff * skb, struct netlink_callback *cb);\n \tint\t\t(*done)(struct netlink_callback *cb);\n \tint\t\tfamily;\n-\tlong\t\targs[6];\n+\tlong\t\targs[7];\n };\n \n struct netlink_notify\ndiff --git a/include/net/xfrm.h b/include/net/xfrm.h\nindex 4bb9499..48630b2 100644\n--- a/include/net/xfrm.h\n+++ b/include/net/xfrm.h\n@@ -1246,6 +1246,8 @@ struct xfrm6_tunnel {\n };\n \n struct xfrm_state_walk {\n+\tstruct list_head list;\n+\tunsigned long genid;\n \tstruct xfrm_state *state;\n \tint count;\n \tu8 proto;\n@@ -1281,13 +1283,7 @@ static inline void xfrm6_fini(void)\n extern int xfrm_proc_init(void);\n #endif\n \n-static inline void xfrm_state_walk_init(struct xfrm_state_walk *walk, u8 proto)\n-{\n-\twalk->proto = proto;\n-\twalk->state = NULL;\n-\twalk->count = 0;\n-}\n-\n+extern void xfrm_state_walk_init(struct xfrm_state_walk *walk, u8 proto);\n extern int xfrm_state_walk(struct xfrm_state_walk *walk,\n \t\t\t   int (*func)(struct xfrm_state *, int, void*), void *);\n extern void xfrm_state_walk_done(struct xfrm_state_walk *walk);\ndiff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c\nindex fe84a46..f7805c5 100644\n--- a/net/xfrm/xfrm_state.c\n+++ b/net/xfrm/xfrm_state.c\n@@ -64,6 +64,9 @@ static unsigned long xfrm_state_walk_ongoing;\n /* Counter indicating walk completion, protected by xfrm_cfg_mutex. */\n static unsigned long xfrm_state_walk_completed;\n \n+/* List of outstanding state walks used to set the completed counter.  */\n+static LIST_HEAD(xfrm_state_walks);\n+\n static struct xfrm_state_afinfo *xfrm_state_get_afinfo(unsigned int family);\n static void xfrm_state_put_afinfo(struct xfrm_state_afinfo *afinfo);\n \n@@ -1582,7 +1585,6 @@ int xfrm_state_walk(struct xfrm_state_walk *walk,\n \t\t\tif (err) {\n \t\t\t\txfrm_state_hold(last);\n \t\t\t\twalk->state = last;\n-\t\t\t\txfrm_state_walk_ongoing++;\n \t\t\t\tgoto out;\n \t\t\t}\n \t\t}\n@@ -1597,25 +1599,44 @@ int xfrm_state_walk(struct xfrm_state_walk *walk,\n \t\terr = func(last, 0, data);\n out:\n \tspin_unlock_bh(&xfrm_state_lock);\n-\tif (old != NULL) {\n+\tif (old != NULL)\n \t\txfrm_state_put(old);\n-\t\txfrm_state_walk_completed++;\n-\t\tif (!list_empty(&xfrm_state_gc_leftovers))\n-\t\t\tschedule_work(&xfrm_state_gc_work);\n-\t}\n \treturn err;\n }\n EXPORT_SYMBOL(xfrm_state_walk);\n \n+void xfrm_state_walk_init(struct xfrm_state_walk *walk, u8 proto)\n+{\n+\twalk->proto = proto;\n+\twalk->state = NULL;\n+\twalk->count = 0;\n+\tlist_add_tail(&walk->list, &xfrm_state_walks);\n+\twalk->genid = ++xfrm_state_walk_ongoing;\n+}\n+EXPORT_SYMBOL(xfrm_state_walk_init);\n+\n void xfrm_state_walk_done(struct xfrm_state_walk *walk)\n {\n+\tstruct list_head *prev;\n+\n \tif (walk->state != NULL) {\n \t\txfrm_state_put(walk->state);\n \t\twalk->state = NULL;\n-\t\txfrm_state_walk_completed++;\n-\t\tif (!list_empty(&xfrm_state_gc_leftovers))\n-\t\t\tschedule_work(&xfrm_state_gc_work);\n \t}\n+\n+\tprev = walk->list.prev;\n+\tlist_del(&walk->list);\n+\n+\tif (prev != &xfrm_state_walks) {\n+\t\tlist_entry(prev, struct xfrm_state_walk, list)->genid =\n+\t\t\twalk->genid;\n+\t\treturn;\n+\t}\n+\n+\txfrm_state_walk_completed = walk->genid;\n+\n+\tif (!list_empty(&xfrm_state_gc_leftovers))\n+\t\tschedule_work(&xfrm_state_gc_work);\n }\n EXPORT_SYMBOL(xfrm_state_walk_done);\n \n",
    "prefixes": []
}