lkml.org 
[lkml]   [2017]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.9 85/88] nl80211: fix dumpit error path RTNL deadlocks
    Date
    4.9-stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Johannes Berg <johannes.berg@intel.com>

    commit ea90e0dc8cecba6359b481e24d9c37160f6f524f upstream.

    Sowmini pointed out Dmitry's RTNL deadlock report to me, and it turns out
    to be perfectly accurate - there are various error paths that miss unlock
    of the RTNL.

    To fix those, change the locking a bit to not be conditional in all those
    nl80211_prepare_*_dump() functions, but make those require the RTNL to
    start with, and fix the buggy error paths. This also let me use sparse
    (by appropriately overriding the rtnl_lock/rtnl_unlock functions) to
    validate the changes.

    Reported-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: Johannes Berg <johannes.berg@intel.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    net/wireless/nl80211.c | 133 ++++++++++++++++++++++---------------------------
    1 file changed, 60 insertions(+), 73 deletions(-)

    --- a/net/wireless/nl80211.c
    +++ b/net/wireless/nl80211.c
    @@ -548,21 +548,17 @@ static int nl80211_prepare_wdev_dump(str
    {
    int err;

    - rtnl_lock();
    -
    if (!cb->args[0]) {
    err = nlmsg_parse(cb->nlh, GENL_HDRLEN + nl80211_fam.hdrsize,
    nl80211_fam.attrbuf, nl80211_fam.maxattr,
    nl80211_policy);
    if (err)
    - goto out_unlock;
    + return err;

    *wdev = __cfg80211_wdev_from_attrs(sock_net(skb->sk),
    nl80211_fam.attrbuf);
    - if (IS_ERR(*wdev)) {
    - err = PTR_ERR(*wdev);
    - goto out_unlock;
    - }
    + if (IS_ERR(*wdev))
    + return PTR_ERR(*wdev);
    *rdev = wiphy_to_rdev((*wdev)->wiphy);
    /* 0 is the first index - add 1 to parse only once */
    cb->args[0] = (*rdev)->wiphy_idx + 1;
    @@ -572,10 +568,8 @@ static int nl80211_prepare_wdev_dump(str
    struct wiphy *wiphy = wiphy_idx_to_wiphy(cb->args[0] - 1);
    struct wireless_dev *tmp;

    - if (!wiphy) {
    - err = -ENODEV;
    - goto out_unlock;
    - }
    + if (!wiphy)
    + return -ENODEV;
    *rdev = wiphy_to_rdev(wiphy);
    *wdev = NULL;

    @@ -586,21 +580,11 @@ static int nl80211_prepare_wdev_dump(str
    }
    }

    - if (!*wdev) {
    - err = -ENODEV;
    - goto out_unlock;
    - }
    + if (!*wdev)
    + return -ENODEV;
    }

    return 0;
    - out_unlock:
    - rtnl_unlock();
    - return err;
    -}
    -
    -static void nl80211_finish_wdev_dump(struct cfg80211_registered_device *rdev)
    -{
    - rtnl_unlock();
    }

    /* IE validation */
    @@ -2584,17 +2568,17 @@ static int nl80211_dump_interface(struct
    int filter_wiphy = -1;
    struct cfg80211_registered_device *rdev;
    struct wireless_dev *wdev;
    + int ret;

    rtnl_lock();
    if (!cb->args[2]) {
    struct nl80211_dump_wiphy_state state = {
    .filter_wiphy = -1,
    };
    - int ret;

    ret = nl80211_dump_wiphy_parse(skb, cb, &state);
    if (ret)
    - return ret;
    + goto out_unlock;

    filter_wiphy = state.filter_wiphy;

    @@ -2639,12 +2623,14 @@ static int nl80211_dump_interface(struct
    wp_idx++;
    }
    out:
    - rtnl_unlock();
    -
    cb->args[0] = wp_idx;
    cb->args[1] = if_idx;

    - return skb->len;
    + ret = skb->len;
    + out_unlock:
    + rtnl_unlock();
    +
    + return ret;
    }

    static int nl80211_get_interface(struct sk_buff *skb, struct genl_info *info)
    @@ -4371,9 +4357,10 @@ static int nl80211_dump_station(struct s
    int sta_idx = cb->args[2];
    int err;

    + rtnl_lock();
    err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev);
    if (err)
    - return err;
    + goto out_err;

    if (!wdev->netdev) {
    err = -EINVAL;
    @@ -4408,7 +4395,7 @@ static int nl80211_dump_station(struct s
    cb->args[2] = sta_idx;
    err = skb->len;
    out_err:
    - nl80211_finish_wdev_dump(rdev);
    + rtnl_unlock();

    return err;
    }
    @@ -5179,9 +5166,10 @@ static int nl80211_dump_mpath(struct sk_
    int path_idx = cb->args[2];
    int err;

    + rtnl_lock();
    err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev);
    if (err)
    - return err;
    + goto out_err;

    if (!rdev->ops->dump_mpath) {
    err = -EOPNOTSUPP;
    @@ -5214,7 +5202,7 @@ static int nl80211_dump_mpath(struct sk_
    cb->args[2] = path_idx;
    err = skb->len;
    out_err:
    - nl80211_finish_wdev_dump(rdev);
    + rtnl_unlock();
    return err;
    }

    @@ -5374,9 +5362,10 @@ static int nl80211_dump_mpp(struct sk_bu
    int path_idx = cb->args[2];
    int err;

    + rtnl_lock();
    err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev);
    if (err)
    - return err;
    + goto out_err;

    if (!rdev->ops->dump_mpp) {
    err = -EOPNOTSUPP;
    @@ -5409,7 +5398,7 @@ static int nl80211_dump_mpp(struct sk_bu
    cb->args[2] = path_idx;
    err = skb->len;
    out_err:
    - nl80211_finish_wdev_dump(rdev);
    + rtnl_unlock();
    return err;
    }

    @@ -7556,9 +7545,12 @@ static int nl80211_dump_scan(struct sk_b
    int start = cb->args[2], idx = 0;
    int err;

    + rtnl_lock();
    err = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev);
    - if (err)
    + if (err) {
    + rtnl_unlock();
    return err;
    + }

    wdev_lock(wdev);
    spin_lock_bh(&rdev->bss_lock);
    @@ -7581,7 +7573,7 @@ static int nl80211_dump_scan(struct sk_b
    wdev_unlock(wdev);

    cb->args[2] = idx;
    - nl80211_finish_wdev_dump(rdev);
    + rtnl_unlock();

    return skb->len;
    }
    @@ -7665,9 +7657,10 @@ static int nl80211_dump_survey(struct sk
    int res;
    bool radio_stats;

    + rtnl_lock();
    res = nl80211_prepare_wdev_dump(skb, cb, &rdev, &wdev);
    if (res)
    - return res;
    + goto out_err;

    /* prepare_wdev_dump parsed the attributes */
    radio_stats = nl80211_fam.attrbuf[NL80211_ATTR_SURVEY_RADIO_STATS];
    @@ -7708,7 +7701,7 @@ static int nl80211_dump_survey(struct sk
    cb->args[2] = survey_idx;
    res = skb->len;
    out_err:
    - nl80211_finish_wdev_dump(rdev);
    + rtnl_unlock();
    return res;
    }

    @@ -11299,17 +11292,13 @@ static int nl80211_prepare_vendor_dump(s
    void *data = NULL;
    unsigned int data_len = 0;

    - rtnl_lock();
    -
    if (cb->args[0]) {
    /* subtract the 1 again here */
    struct wiphy *wiphy = wiphy_idx_to_wiphy(cb->args[0] - 1);
    struct wireless_dev *tmp;

    - if (!wiphy) {
    - err = -ENODEV;
    - goto out_unlock;
    - }
    + if (!wiphy)
    + return -ENODEV;
    *rdev = wiphy_to_rdev(wiphy);
    *wdev = NULL;

    @@ -11330,13 +11319,11 @@ static int nl80211_prepare_vendor_dump(s
    nl80211_fam.attrbuf, nl80211_fam.maxattr,
    nl80211_policy);
    if (err)
    - goto out_unlock;
    + return err;

    if (!nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_ID] ||
    - !nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_SUBCMD]) {
    - err = -EINVAL;
    - goto out_unlock;
    - }
    + !nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_SUBCMD])
    + return -EINVAL;

    *wdev = __cfg80211_wdev_from_attrs(sock_net(skb->sk),
    nl80211_fam.attrbuf);
    @@ -11345,10 +11332,8 @@ static int nl80211_prepare_vendor_dump(s

    *rdev = __cfg80211_rdev_from_attrs(sock_net(skb->sk),
    nl80211_fam.attrbuf);
    - if (IS_ERR(*rdev)) {
    - err = PTR_ERR(*rdev);
    - goto out_unlock;
    - }
    + if (IS_ERR(*rdev))
    + return PTR_ERR(*rdev);

    vid = nla_get_u32(nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_ID]);
    subcmd = nla_get_u32(nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_SUBCMD]);
    @@ -11361,19 +11346,15 @@ static int nl80211_prepare_vendor_dump(s
    if (vcmd->info.vendor_id != vid || vcmd->info.subcmd != subcmd)
    continue;

    - if (!vcmd->dumpit) {
    - err = -EOPNOTSUPP;
    - goto out_unlock;
    - }
    + if (!vcmd->dumpit)
    + return -EOPNOTSUPP;

    vcmd_idx = i;
    break;
    }

    - if (vcmd_idx < 0) {
    - err = -EOPNOTSUPP;
    - goto out_unlock;
    - }
    + if (vcmd_idx < 0)
    + return -EOPNOTSUPP;

    if (nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_DATA]) {
    data = nla_data(nl80211_fam.attrbuf[NL80211_ATTR_VENDOR_DATA]);
    @@ -11390,9 +11371,6 @@ static int nl80211_prepare_vendor_dump(s

    /* keep rtnl locked in successful case */
    return 0;
    - out_unlock:
    - rtnl_unlock();
    - return err;
    }

    static int nl80211_vendor_cmd_dump(struct sk_buff *skb,
    @@ -11407,9 +11385,10 @@ static int nl80211_vendor_cmd_dump(struc
    int err;
    struct nlattr *vendor_data;

    + rtnl_lock();
    err = nl80211_prepare_vendor_dump(skb, cb, &rdev, &wdev);
    if (err)
    - return err;
    + goto out;

    vcmd_idx = cb->args[2];
    data = (void *)cb->args[3];
    @@ -11418,18 +11397,26 @@ static int nl80211_vendor_cmd_dump(struc

    if (vcmd->flags & (WIPHY_VENDOR_CMD_NEED_WDEV |
    WIPHY_VENDOR_CMD_NEED_NETDEV)) {
    - if (!wdev)
    - return -EINVAL;
    + if (!wdev) {
    + err = -EINVAL;
    + goto out;
    + }
    if (vcmd->flags & WIPHY_VENDOR_CMD_NEED_NETDEV &&
    - !wdev->netdev)
    - return -EINVAL;
    + !wdev->netdev) {
    + err = -EINVAL;
    + goto out;
    + }

    if (vcmd->flags & WIPHY_VENDOR_CMD_NEED_RUNNING) {
    if (wdev->netdev &&
    - !netif_running(wdev->netdev))
    - return -ENETDOWN;
    - if (!wdev->netdev && !wdev->p2p_started)
    - return -ENETDOWN;
    + !netif_running(wdev->netdev)) {
    + err = -ENETDOWN;
    + goto out;
    + }
    + if (!wdev->netdev && !wdev->p2p_started) {
    + err = -ENETDOWN;
    + goto out;
    + }
    }
    }


    \
     
     \ /
      Last update: 2017-03-28 14:56    [W:4.076 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site