Cleanup AP logic after Hostapd is down

Bug: 35445677
Test: compile, unit tests, manual test

Change-Id: Ic7e6c593ded19955da7a85d60730bf8e9afd679a

diff --git a/ap_interface_impl.cpp b/ap_interface_impl.cpp
index e7f0df9..62fa2ae 100644 (file)
--- a/ap_interface_impl.cpp
+++ b/ap_interface_impl.cpp
@@ -18,6 +18,8 @@
 
 #include <android-base/logging.h>
 
+#include "wificond/net/netlink_utils.h"
+
 #include "wificond/ap_interface_binder.h"
 
 using android::net::wifi::IApInterface;
@@ -34,10 +36,12 @@ namespace wificond {
 
 ApInterfaceImpl::ApInterfaceImpl(const string& interface_name,
                                  uint32_t interface_index,
+                                 NetlinkUtils* netlink_utils,
                                  InterfaceTool* if_tool,
                                  HostapdManager* hostapd_manager)
     : interface_name_(interface_name),
       interface_index_(interface_index),
+      netlink_utils_(netlink_utils),
       if_tool_(if_tool),
       hostapd_manager_(hostapd_manager),
       binder_(new ApInterfaceBinder(this)) {
@@ -61,14 +65,28 @@ bool ApInterfaceImpl::StartHostapd() {
 
 bool ApInterfaceImpl::StopHostapd() {
   // Drop SIGKILL on hostapd.
-  bool success = hostapd_manager_->StopHostapd();
+  if (!hostapd_manager_->StopHostapd()) {
+    // Logging was done internally.
+    return false;
+  }
+
+  // Take down the interface.
+  if (!if_tool_->SetUpState(interface_name_.c_str(), false)) {
+    // Logging was done internally.
+    return false;
+  }
 
-  // Take down the interface.  This has the pleasant side effect of
-  // letting the driver know that we don't want any lingering AP logic
-  // running in the driver.
-  success = if_tool_->SetUpState(interface_name_.c_str(), false) && success;
+  // Since wificond SIGKILLs hostapd, hostapd has no chance to handle
+  // the cleanup.
+  // Besides taking down the interface, we also need to set the interface mode
+  // back to station mode for the cleanup.
+  if (!netlink_utils_->SetInterfaceMode(interface_index_,
+                                        NetlinkUtils::STATION_MODE)) {
+    LOG(ERROR) << "Failed to set interface back to station mode";
+    return false;
+  }
 
-  return success;
+  return true;
 }
 
 bool ApInterfaceImpl::WriteHostapdConfig(const vector<uint8_t>& ssid,

diff --git a/ap_interface_impl.h b/ap_interface_impl.h
index 5e8eae6..de6af5d 100644 (file)
--- a/ap_interface_impl.h
+++ b/ap_interface_impl.h
@@ -30,6 +30,7 @@ namespace android {
 namespace wificond {
 
 class ApInterfaceBinder;
+class NetlinkUtils;
 
 // Holds the guts of how we control network interfaces capable of exposing an AP
 // via hostapd.  Because remote processes may hold on to the corresponding
@@ -39,6 +40,7 @@ class ApInterfaceImpl {
  public:
   ApInterfaceImpl(const std::string& interface_name,
                   uint32_t interface_index,
+                  NetlinkUtils* netlink_utils,
                   wifi_system::InterfaceTool* if_tool,
                   wifi_system::HostapdManager* hostapd_manager);
   ~ApInterfaceImpl();
@@ -59,6 +61,7 @@ class ApInterfaceImpl {
  private:
   const std::string interface_name_;
   const uint32_t interface_index_;
+  NetlinkUtils* const netlink_utils_;
   wifi_system::InterfaceTool* const if_tool_;
   wifi_system::HostapdManager* const hostapd_manager_;
   const android::sp<ApInterfaceBinder> binder_;

diff --git a/net/netlink_utils.cpp b/net/netlink_utils.cpp
index 1b8d2a3..459f22b 100644 (file)
--- a/net/netlink_utils.cpp
+++ b/net/netlink_utils.cpp
@@ -172,6 +172,36 @@ bool NetlinkUtils::GetInterfaceInfo(uint32_t wiphy_index,
   return false;
 }
 
+bool NetlinkUtils::SetInterfaceMode(uint32_t interface_index,
+                                    InterfaceMode mode) {
+  uint32_t set_to_mode = NL80211_IFTYPE_UNSPECIFIED;
+  if (mode == STATION_MODE) {
+    set_to_mode = NL80211_IFTYPE_STATION;
+  } else {
+    LOG(ERROR) << "Unexpected mode for interface with index: "
+               << interface_index;
+    return false;
+  }
+  NL80211Packet set_interface_mode(
+      netlink_manager_->GetFamilyId(),
+      NL80211_CMD_SET_INTERFACE,
+      netlink_manager_->GetSequenceNumber(),
+      getpid());
+  // Force an ACK response upon success.
+  set_interface_mode.AddFlag(NLM_F_ACK);
+
+  set_interface_mode.AddAttribute(
+      NL80211Attr<uint32_t>(NL80211_ATTR_IFINDEX, interface_index));
+  set_interface_mode.AddAttribute(
+      NL80211Attr<uint32_t>(NL80211_ATTR_IFTYPE, set_to_mode));
+
+  if (!netlink_manager_->SendMessageAndGetAck(set_interface_mode)) {
+    LOG(ERROR) << "NL80211_CMD_SET_INTERFACE failed";
+    return false;
+  }
+
+  return true;
+}
 
 bool NetlinkUtils::GetWiphyInfo(
     uint32_t wiphy_index,

diff --git a/net/netlink_utils.h b/net/netlink_utils.h
index dbdb00a..65d6335 100644 (file)
--- a/net/netlink_utils.h
+++ b/net/netlink_utils.h
@@ -109,6 +109,12 @@ class NL80211Packet;
 // Provides NL80211 helper functions.
 class NetlinkUtils {
  public:
+  // Currently we only support setting the interface to STATION mode.
+  // This is used for cleaning up interface after KILLING hostapd.
+  enum InterfaceMode{
+      STATION_MODE
+  };
+
   explicit NetlinkUtils(NetlinkManager* netlink_manager);
   virtual ~NetlinkUtils();
 
@@ -125,6 +131,13 @@ class NetlinkUtils {
                                 uint32_t* index,
                                 std::vector<uint8_t>* mac_addr);
 
+  // Set the mode of interface.
+  // |interface_index| is the interface index.
+  // |mode| is one of the values in |enum InterfaceMode|.
+  // Returns true on success.
+  virtual bool SetInterfaceMode(uint32_t interface_index,
+                                InterfaceMode mode);
+
   // Get wiphy capability information from kernel.
   // Returns true on success.
   virtual bool GetWiphyInfo(uint32_t wiphy_index,

diff --git a/server.cpp b/server.cpp
index dca3002..afaba8e 100644 (file)
--- a/server.cpp
+++ b/server.cpp
@@ -122,6 +122,7 @@ Status Server::createApInterface(sp<IApInterface>* created_interface) {
   unique_ptr<ApInterfaceImpl> ap_interface(new ApInterfaceImpl(
       interface_name,
       interface_index,
+      netlink_utils_,
       if_tool_.get(),
       hostapd_manager_.get()));
   *created_interface = ap_interface->GetBinder();

diff --git a/tests/ap_interface_impl_unittest.cpp b/tests/ap_interface_impl_unittest.cpp
index 6ed328e..9f57d41 100644 (file)
--- a/tests/ap_interface_impl_unittest.cpp
+++ b/tests/ap_interface_impl_unittest.cpp
@@ -22,6 +22,9 @@
 #include <wifi_system_test/mock_hostapd_manager.h>
 #include <wifi_system_test/mock_interface_tool.h>
 
+#include "wificond/tests/mock_netlink_manager.h"
+#include "wificond/tests/mock_netlink_utils.h"
+
 #include "wificond/ap_interface_impl.h"
 
 using android::wifi_system::HostapdManager;
@@ -48,8 +51,14 @@ class ApInterfaceImplTest : public ::testing::Test {
       new NiceMock<MockInterfaceTool>};
   unique_ptr<NiceMock<MockHostapdManager>> hostapd_manager_{
       new NiceMock<MockHostapdManager>};
+  unique_ptr<NiceMock<MockNetlinkManager>> netlink_manager_{
+      new NiceMock<MockNetlinkManager>()};
+  unique_ptr<NiceMock<MockNetlinkUtils>> netlink_utils_{
+      new NiceMock<MockNetlinkUtils>(netlink_manager_.get())};
+
   ApInterfaceImpl ap_interface_{kTestInterfaceName,
                                 kTestInterfaceIndex,
+                                netlink_utils_.get(),
                                 if_tool_.get(),
                                 hostapd_manager_.get()};
 };  // class ApInterfaceImplTest
@@ -79,6 +88,9 @@ TEST_F(ApInterfaceImplTest, ShouldReportStopSuccess) {
       .WillOnce(Return(true));
   EXPECT_CALL(*if_tool_, SetUpState(StrEq(kTestInterfaceName), false))
       .WillOnce(Return(true));
+  EXPECT_CALL(*netlink_utils_, SetInterfaceMode(
+      kTestInterfaceIndex,
+      NetlinkUtils::STATION_MODE)).WillOnce(Return(true));
   EXPECT_TRUE(ap_interface_.StopHostapd());
   testing::Mock::VerifyAndClearExpectations(if_tool_.get());
 }

diff --git a/tests/mock_netlink_utils.h b/tests/mock_netlink_utils.h
index 9eded29..c0550a3 100644 (file)
--- a/tests/mock_netlink_utils.h
+++ b/tests/mock_netlink_utils.h
@@ -32,6 +32,8 @@ class MockNetlinkUtils : public NetlinkUtils {
   MOCK_METHOD1(GetWiphyIndex, bool(uint32_t* out_wiphy_index));
   MOCK_METHOD1(UnsubscribeMlmeEvent, void(uint32_t interface_index));
   MOCK_METHOD1(UnsubscribeRegDomainChange, void(uint32_t wiphy_index));
+  MOCK_METHOD2(SetInterfaceMode,
+               bool(uint32_t interface_index, InterfaceMode mode));
   MOCK_METHOD2(SubscribeMlmeEvent,
                void(uint32_t interface_index,
                     MlmeEventHandler* handler));

diff --git a/tests/netlink_utils_unittest.cpp b/tests/netlink_utils_unittest.cpp
index 60d4cc2..c1a01c2 100644 (file)
--- a/tests/netlink_utils_unittest.cpp
+++ b/tests/netlink_utils_unittest.cpp
@@ -74,6 +74,10 @@ NL80211Packet CreateControlMessageError(int error_code) {
   return NL80211Packet(data);
 }
 
+NL80211Packet CreateControlMessageAck() {
+  return CreateControlMessageError(0);
+}
+
 }  // namespace
 
 class NetlinkUtilsTest : public ::testing::Test {
@@ -132,6 +136,28 @@ TEST_F(NetlinkUtilsTest, CanHandleGetWiphyIndexError) {
   EXPECT_FALSE(netlink_utils_->GetWiphyIndex(&wiphy_index));
 }
 
+TEST_F(NetlinkUtilsTest, CanSetIntrerfaceMode) {
+  // Mock a ACK response from kernel.
+  vector<NL80211Packet> response = {CreateControlMessageAck()};
+
+  EXPECT_CALL(*netlink_manager_, SendMessageAndGetResponses(_, _)).
+      WillOnce(DoAll(MakeupResponse(response), Return(true)));
+
+  EXPECT_TRUE(netlink_utils_->SetInterfaceMode(kFakeInterfaceIndex,
+                                               NetlinkUtils::STATION_MODE));
+}
+
+TEST_F(NetlinkUtilsTest, CanHandleSetIntrerfaceModeError) {
+  // Mock an error response from kernel.
+  vector<NL80211Packet> response = {CreateControlMessageError(kFakeErrorCode)};
+
+  EXPECT_CALL(*netlink_manager_, SendMessageAndGetResponses(_, _)).
+      WillOnce(DoAll(MakeupResponse(response), Return(true)));
+
+  EXPECT_FALSE(netlink_utils_->SetInterfaceMode(kFakeInterfaceIndex,
+                                                NetlinkUtils::STATION_MODE));
+}
+
 TEST_F(NetlinkUtilsTest, CanGetInterfaceInfo) {
   NL80211Packet new_interface(
       netlink_manager_->GetFamilyId(),