MIME-Version: 1.0
In-Reply-To: <1483544433.7312.13.camel@sipsolutions.net>
References: <20161218002554.6362-1-andrew.zaborowski@intel.com> <1483544433.7312.13.camel@sipsolutions.net>
From: Andrew Zaborowski <andrew.zaborowski@intel.com>
Date: Wed, 4 Jan 2017 15:35:10 -0500
Message-ID: <CAOq732KZrpsjK96uwwe3hna2DPkwbN3CvJz+LFxJzXxWw08wRA@mail.gmail.com> (sfid-20170104_213515_572455_0998A0A9)
Subject: Re: [PATCH v4] cfg80211: NL80211_ATTR_SOCKET_OWNER support for CMD_CONNECT
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Content-Type: text/plain; charset=UTF-8
Sender: linux-wireless-owner@vger.kernel.org

On 4 January 2017 at 10:40, Johannes Berg <johannes@sipsolutions.net> wrote:
>> +++ b/net/wireless/mlme.c
>> @@ -340,6 +340,8 @@ int cfg80211_mlme_deauth(struct
>> cfg80211_registered_device *rdev,
>>
>>       ASSERT_WDEV_LOCK(wdev);
>>
>> +     wdev->conn_owner_nlportid = 0;
>
> Is this really correct? The deauth might not be to the current_bss, as
> you can see in the following if statement:
>
>>       if (local_state_change &&
>>           (!wdev->current_bss ||
>>            !ether_addr_equal(wdev->current_bss->pub.bssid, bssid)))
>
> It seems that perhaps this should go into some other place, perhaps
> only be reset when current_bss is also reset to NULL?

In this case yes, I think I should perform the same bssid comparison.
But elsewhere we want conn_owner_nlportid to be set earlier than
current_bss, and reset earlier than current_bss because (1) we want to
be able to interrupt an ongoing attempt, and (2) we also don't want to
trigger another disconnect / deauth if one is already in progress.

>
>> @@ -14539,13 +14554,21 @@ static int nl80211_netlink_notify(struct
>> notifier_block * nb,
>>                               spin_unlock(&rdev-
>> >destroy_list_lock);
>>                               schedule_work(&rdev->destroy_work);
>>                       }
>> -             } else if (schedule_scan_stop) {
>> +
>> +                     continue;
>> +             }
>
> This also doesn't seem right - the same socket could possibly own both
> an interface and a connection? If the connection is on the same
> interface you might not really want to do both - though it shouldn't
> hurt if all the cancel_work is in the right place - but it could be a
> different interface?

This is only a syntactic change though.  The "continue" is now in the
"if (schedule_destroy_work)" block so the other actions will not be
scheduled is the interface is being destroyed.

Best regards