wifi: mac80211: keep mac80211 consistent on link activation failure

In the unlikely event that link_use_channel fails while activating a
link, mac80211 would go into a bad state. Unfortunately, we cannot
completely avoid failures from drivers in this case.

However, what we can do is to just continue internally anyway and assume
the driver is going to trigger a recovery flow from its side. Doing that
means that we at least have a consistent state in mac80211 allowing such
a recovery flow to succeed.

Reviewed-by: Miriam Rachel Korenblit <miriam.rachel.korenblit@intel.com>
Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Link: https://msgid.link/20240418115219.1129e89f4b55.I6299678353e50e88b55c99b0bce15c64b52c2804@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 2c338014ded582400b04a8b3be15a718c8162861..dedf11eeb43c672f4b56633e63997603d6006c94 100644 (file)
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -688,7 +688,8 @@ static int ieee80211_add_chanctx(struct ieee80211_local *local,
 static struct ieee80211_chanctx *
 ieee80211_new_chanctx(struct ieee80211_local *local,
                      const struct ieee80211_chan_req *chanreq,
-                     enum ieee80211_chanctx_mode mode)
+                     enum ieee80211_chanctx_mode mode,
+                     bool assign_on_failure)
 {
        struct ieee80211_chanctx *ctx;
        int err;
@@ -700,10 +701,12 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
                return ERR_PTR(-ENOMEM);
 
        err = ieee80211_add_chanctx(local, ctx);
-       if (err) {
+       if (!assign_on_failure && err) {
                kfree(ctx);
                return ERR_PTR(err);
        }
+       /* We ignored a driver error, see _ieee80211_set_active_links */
+       WARN_ON_ONCE(err && !local->in_reconfig);
 
        list_add_rcu(&ctx->list, &local->chanctx_list);
        return ctx;
@@ -809,7 +812,8 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
 }
 
 static int ieee80211_assign_link_chanctx(struct ieee80211_link_data *link,
-                                        struct ieee80211_chanctx *new_ctx)
+                                        struct ieee80211_chanctx *new_ctx,
+                                        bool assign_on_failure)
 {
        struct ieee80211_sub_if_data *sdata = link->sdata;
        struct ieee80211_local *local = sdata->local;
@@ -836,7 +840,11 @@ static int ieee80211_assign_link_chanctx(struct ieee80211_link_data *link,
                ieee80211_recalc_chanctx_min_def(local, new_ctx, link);
 
                ret = drv_assign_vif_chanctx(local, sdata, link->conf, new_ctx);
-               if (!ret) {
+               if (assign_on_failure || !ret) {
+                       /* Need to continue, see _ieee80211_set_active_links */
+                       WARN_ON_ONCE(ret && !local->in_reconfig);
+                       ret = 0;
+
                        /* succeeded, so commit it to the data structures */
                        conf = &new_ctx->conf;
                        list_add(&link->assigned_chanctx_list,
@@ -1046,7 +1054,8 @@ int ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
        new_ctx = ieee80211_find_reservation_chanctx(local, chanreq, mode);
        if (!new_ctx) {
                if (ieee80211_can_create_new_chanctx(local)) {
-                       new_ctx = ieee80211_new_chanctx(local, chanreq, mode);
+                       new_ctx = ieee80211_new_chanctx(local, chanreq, mode,
+                                                       false);
                        if (IS_ERR(new_ctx))
                                return PTR_ERR(new_ctx);
                } else {
@@ -1302,7 +1311,7 @@ ieee80211_link_use_reserved_assign(struct ieee80211_link_data *link)
        list_del(&link->reserved_chanctx_list);
        link->reserved_chanctx = NULL;
 
-       err = ieee80211_assign_link_chanctx(link, new_ctx);
+       err = ieee80211_assign_link_chanctx(link, new_ctx, false);
        if (err) {
                if (ieee80211_chanctx_refcount(local, new_ctx) == 0)
                        ieee80211_free_chanctx(local, new_ctx, false);
@@ -1698,7 +1707,7 @@ void __ieee80211_link_release_channel(struct ieee80211_link_data *link,
                ieee80211_link_unreserve_chanctx(link);
        }
 
-       ieee80211_assign_link_chanctx(link, NULL);
+       ieee80211_assign_link_chanctx(link, NULL, false);
        if (ieee80211_chanctx_refcount(local, ctx) == 0)
                ieee80211_free_chanctx(local, ctx, skip_idle_recalc);
 
@@ -1709,9 +1718,10 @@ void __ieee80211_link_release_channel(struct ieee80211_link_data *link,
                ieee80211_vif_use_reserved_switch(local);
 }
 
-int ieee80211_link_use_channel(struct ieee80211_link_data *link,
-                              const struct ieee80211_chan_req *chanreq,
-                              enum ieee80211_chanctx_mode mode)
+int _ieee80211_link_use_channel(struct ieee80211_link_data *link,
+                               const struct ieee80211_chan_req *chanreq,
+                               enum ieee80211_chanctx_mode mode,
+                               bool assign_on_failure)
 {
        struct ieee80211_sub_if_data *sdata = link->sdata;
        struct ieee80211_local *local = sdata->local;
@@ -1749,7 +1759,8 @@ int ieee80211_link_use_channel(struct ieee80211_link_data *link,
        if (ctx)
                reserved = true;
        else
-               ctx = ieee80211_new_chanctx(local, chanreq, mode);
+               ctx = ieee80211_new_chanctx(local, chanreq, mode,
+                                           assign_on_failure);
        if (IS_ERR(ctx)) {
                ret = PTR_ERR(ctx);
                goto out;
@@ -1757,7 +1768,7 @@ int ieee80211_link_use_channel(struct ieee80211_link_data *link,
 
        ieee80211_link_update_chanreq(link, chanreq);
 
-       ret = ieee80211_assign_link_chanctx(link, ctx);
+       ret = ieee80211_assign_link_chanctx(link, ctx, assign_on_failure);
 
        if (reserved) {
                /* remove reservation */

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 5c95ec9f499f4080fd4d8e055d982b24dc230303..eb62b7d4b4f7e298ce70b87c45fac16a28a03c94 100644 (file)
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -2554,9 +2554,19 @@ bool ieee80211_chanreq_identical(const struct ieee80211_chan_req *a,
                                 const struct ieee80211_chan_req *b);
 
 int __must_check
+_ieee80211_link_use_channel(struct ieee80211_link_data *link,
+                           const struct ieee80211_chan_req *req,
+                           enum ieee80211_chanctx_mode mode,
+                           bool assign_on_failure);
+
+static inline int __must_check
 ieee80211_link_use_channel(struct ieee80211_link_data *link,
                           const struct ieee80211_chan_req *req,
-                          enum ieee80211_chanctx_mode mode);
+                          enum ieee80211_chanctx_mode mode)
+{
+       return _ieee80211_link_use_channel(link, req, mode, false);
+}
+
 int __must_check
 ieee80211_link_reserve_chanctx(struct ieee80211_link_data *link,
                               const struct ieee80211_chan_req *req,

diff --git a/net/mac80211/link.c b/net/mac80211/link.c
index 43f9672fc7f19cc0c91751ee627312f4a5425a70..af0321408a97af1bf1dccd5eccc4bfb502d599d7 100644 (file)
--- a/net/mac80211/link.c
+++ b/net/mac80211/link.c
@@ -404,9 +404,24 @@ static int _ieee80211_set_active_links(struct ieee80211_sub_if_data *sdata,
 
                link = sdata_dereference(sdata->link[link_id], sdata);
 
-               ret = ieee80211_link_use_channel(link,
-                                                &link->conf->chanreq,
-                                                IEEE80211_CHANCTX_SHARED);
+               /*
+                * This call really should not fail. Unfortunately, it appears
+                * that this may happen occasionally with some drivers. Should
+                * it happen, we are stuck in a bad place as going backwards is
+                * not really feasible.
+                *
+                * So lets just tell link_use_channel that it must not fail to
+                * assign the channel context (from mac80211's perspective) and
+                * assume the driver is going to trigger a recovery flow if it
+                * had a failure.
+                * That really is not great nor guaranteed to work. But at least
+                * the internal mac80211 state remains consistent and there is
+                * a chance that we can recover.
+                */
+               ret = _ieee80211_link_use_channel(link,
+                                                 &link->conf->chanreq,
+                                                 IEEE80211_CHANCTX_SHARED,
+                                                 true);
                WARN_ON_ONCE(ret);
 
                ieee80211_mgd_set_link_qos_params(link);