Merge openenclave#2725 openenclave#2741

2725: Fix fd not being deleted from epoll on close r=jhand2 a=thomasten If an fd is closed, it is implicitly deleted from all epoll instances by the system. The enclave epoll mapping must also be updated. 2741: Use private IPs when building Windows images r=BRMcLaren a=oprinmarius Use private IPs when building Windows Images Co-authored-by: Thomas Tendyck <[email protected]> Co-authored-by: Oprin Marius <[email protected]>
BRMcLaren · Mar 26, 2020 · 2536d0c · 2536d0c
3 parents aa1ebcb + 11562dd + e66d1f3
commit 2536d0c
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 23 deletions.
diff --git a/.jenkins/build_azure_managed_images.Jenkinsfile b/.jenkins/build_azure_managed_images.Jenkinsfile
@@ -53,7 +53,10 @@ def buildWindowsManagedImage(String os_series, String img_name_suffix, String la
                 } else {
                     managed_image_name_id = sh(script: "git rev-parse --short HEAD", returnStdout: true).tokenize().last()
                 }
-                def rg_name = "build-${managed_image_name_id}-${img_name_suffix}-${BUILD_NUMBER}"
+                def vm_rg_name = "build-${managed_image_name_id}-${img_name_suffix}-${BUILD_NUMBER}"
+                def jenkins_rg_name = params.JENKINS_RESOURCE_GROUP
+                def jenkins_vnet_name = params.JENKINS_VNET_NAME
+                def jenkins_subnet_name = params.JENKINS_SUBNET_NAME
                 def azure_image_id = AZURE_IMAGES_MAP[os_series]
                 withCredentials([usernamePassword(credentialsId: JENKINS_USER_CREDS_ID,
                                                   usernameVariable: "JENKINS_USER_NAME",
@@ -83,55 +86,51 @@ def buildWindowsManagedImage(String os_series, String img_name_suffix, String la
 
                         ${az_login_script}
 
+                        SUBNET_ID=`az network vnet subnet show \
+                            --resource-group ${jenkins_rg_name} \
+                            --name ${jenkins_subnet_name} \
+                            --vnet-name ${jenkins_vnet_name} --query id -o tsv`
+
                         VM_ID=`az vm create \
-                            --resource-group ${rg_name} \
+                            --resource-group ${vm_rg_name} \
                             --location ${REGION} \
                             --name ${img_name_suffix} \
                             --size Standard_DC4s \
                             --os-disk-size-gb 128 \
+                            --subnet \$SUBNET_ID \
                             --admin-username ${JENKINS_USER_NAME} \
                             --admin-password ${JENKINS_USER_PASSWORD} \
                             --image ${azure_image_id} | jq -r '.id'`
 
                         VM_DETAILS=`az vm show --ids \$VM_ID --show-details`
 
-                        NIC_ID=`echo \$VM_DETAILS | jq -r '.networkProfile.networkInterfaces[0].id'`
-                        NSG_ID=`az network nic show --ids \$NIC_ID | jq -r '.networkSecurityGroup.id'`
-                        NSG_NAME=`az network nsg show --ids \$NSG_ID | jq -r '.name'`
-                        az network nsg rule create \
-                            --resource-group ${rg_name} \
-                            --nsg-name \$NSG_NAME \
-                            --name WinRM --priority 500 \
-                            --access Allow --direction Inbound --protocol Tcp \
-                            --source-address-prefixes Internet \
-                            --destination-port-ranges 5986
                         az vm run-command invoke \
-                            --resource-group ${rg_name} \
+                            --resource-group ${vm_rg_name} \
                             --name ${img_name_suffix} \
                             --command-id EnableRemotePS
 
-                        PUBLIC_IP=`echo \$VM_DETAILS | jq -r '.publicIps'`
+                        PRIVATE_IP=`echo \$VM_DETAILS | jq -r '.privateIps'`
                         echo "[windows-agents]" > $WORKSPACE/scripts/ansible/inventory/hosts
-                        echo "\$PUBLIC_IP" >> $WORKSPACE/scripts/ansible/inventory/hosts
-                        echo "ansible_user: ${JENKINS_USER_NAME}" > $WORKSPACE/scripts/ansible/inventory/host_vars/\$PUBLIC_IP
-                        echo "ansible_password: ${JENKINS_USER_PASSWORD}" >> $WORKSPACE/scripts/ansible/inventory/host_vars/\$PUBLIC_IP
-                        echo "ansible_winrm_transport: ntlm" >> $WORKSPACE/scripts/ansible/inventory/host_vars/\$PUBLIC_IP
-                        echo "launch_configuration: ${launch_configuration}" >> $WORKSPACE/scripts/ansible/inventory/host_vars/\$PUBLIC_IP
+                        echo "\$PRIVATE_IP" >> $WORKSPACE/scripts/ansible/inventory/hosts
+                        echo "ansible_user: ${JENKINS_USER_NAME}" > $WORKSPACE/scripts/ansible/inventory/host_vars/\$PRIVATE_IP
+                        echo "ansible_password: ${JENKINS_USER_PASSWORD}" >> $WORKSPACE/scripts/ansible/inventory/host_vars/\$PRIVATE_IP
+                        echo "ansible_winrm_transport: ntlm" >> $WORKSPACE/scripts/ansible/inventory/host_vars/\$PRIVATE_IP
+                        echo "launch_configuration: ${launch_configuration}" >> $WORKSPACE/scripts/ansible/inventory/host_vars/\$PRIVATE_IP
 
                         cd $WORKSPACE/scripts/ansible
                         retrycmd_if_failure 5 10 2h ansible-playbook oe-windows-acc-setup.yml
                         retrycmd_if_failure 5 10 30m ansible-playbook jenkins-packer.yml
 
                         az vm run-command invoke \
-                            --resource-group ${rg_name} \
+                            --resource-group ${vm_rg_name} \
                             --name ${img_name_suffix} \
                             --command-id RunPowerShellScript \
                             --scripts @$WORKSPACE/.jenkins/provision/run-sysprep.ps1
 
                         az vm deallocate --ids \$VM_ID
                         az vm generalize --ids \$VM_ID
                         MANAGED_IMG_ID=`az image create \
-                            --resource-group ${rg_name} \
+                            --resource-group ${vm_rg_name} \
                             --name ${managed_image_name_id}-${img_name_suffix} \
                             --source ${img_name_suffix} | jq -r '.id'`
 
@@ -147,11 +146,11 @@ def buildWindowsManagedImage(String os_series, String img_name_suffix, String la
                     """
                     def az_rg_create_script = """
                         ${az_login_script}
-                        az group create --name ${rg_name} --location ${REGION}
+                        az group create --name ${vm_rg_name} --location ${REGION}
                     """
                     def az_rg_cleanup_script = """
                         ${az_login_script}
-                        az group delete --name ${rg_name} --yes
+                        az group delete --name ${vm_rg_name} --yes
                     """
                     oe.exec_with_retry(10, 300) {
                         try {

diff --git a/include/openenclave/internal/syscall/fd.h b/include/openenclave/internal/syscall/fd.h
@@ -153,6 +153,8 @@ typedef struct _oe_epoll_ops
         struct oe_epoll_event* events,
         int maxevents,
         int timeout);
+
+    void (*on_close)(oe_fd_t* epoll, int fd);
 } oe_epoll_ops_t;
 
 struct _oe_fd

diff --git a/include/openenclave/internal/syscall/fdtable.h b/include/openenclave/internal/syscall/fdtable.h
@@ -18,6 +18,20 @@ int oe_fdtable_reassign(int fd, oe_fd_t* new_desc, oe_fd_t** old_desc);
 
 int oe_fdtable_release(int fd);
 
+/**
+ * Invokes **callback** for each fd of type **type** in the fdtable.
+ *
+ * The callback must not use any of the other fdtable functions.
+ *
+ * @param type The fd type of interest. Can be OE_FD_TYPE_ANY.
+ * @param arg An argument passed to the callback.
+ * @param callback The callback to be invoked.
+ */
+void oe_fdtable_foreach(
+    oe_fd_type_t type,
+    void* arg,
+    void (*callback)(oe_fd_t* desc, void* arg));
+
 OE_EXTERNC_END
 
 #endif // _OE_SYSCALL_FDTABLE_H
diff --git a/syscall/devices/hostepoll/hostepoll.c b/syscall/devices/hostepoll/hostepoll.c
@@ -809,6 +809,29 @@ static int _epoll_dup(oe_fd_t* epoll_, oe_fd_t** new_epoll_out)
     return ret;
 }
 
+static void _epoll_on_close(oe_fd_t* epoll_, int fd)
+{
+    epoll_t* const epoll = _cast_epoll(epoll_);
+    oe_assert(epoll);
+
+    oe_assert(fd >= 0);
+
+    oe_mutex_lock(&epoll->lock);
+
+    /* Delete the mapping if it exists. */
+    for (size_t i = 0; i < epoll->map_size; i++)
+    {
+        if (epoll->map[i].fd == fd)
+        {
+            /* Swap with last element of array. */
+            epoll->map[i] = epoll->map[--epoll->map_size];
+            break;
+        }
+    }
+
+    oe_mutex_unlock(&epoll->lock);
+}
+
 static oe_epoll_ops_t _epoll_ops = {
     .fd.read = _epoll_read,
     .fd.write = _epoll_write,
@@ -821,6 +844,7 @@ static oe_epoll_ops_t _epoll_ops = {
     .fd.get_host_fd = _epoll_get_host_fd,
     .epoll_ctl = _epoll_ctl,
     .epoll_wait = _epoll_wait,
+    .on_close = _epoll_on_close,
 };
 
 static oe_epoll_ops_t _get_epoll_ops(void)

diff --git a/syscall/fdtable.c b/syscall/fdtable.c
@@ -194,6 +194,7 @@ static void _assert_fd(oe_fd_t* desc)
         {
             oe_assert(desc->ops.epoll.epoll_ctl);
             oe_assert(desc->ops.epoll.epoll_wait);
+            oe_assert(desc->ops.epoll.on_close);
             break;
         }
     }
@@ -364,3 +365,22 @@ oe_fd_t* oe_fdtable_get(int fd, oe_fd_type_t type)
 done:
     return ret;
 }
+
+void oe_fdtable_foreach(
+    oe_fd_type_t type,
+    void* arg,
+    void (*callback)(oe_fd_t* desc, void* arg))
+{
+    oe_assert(callback);
+
+    oe_spin_lock(&_lock);
+
+    for (size_t i = 0; i < _table_size; ++i)
+    {
+        oe_fd_t* const desc = _table[i];
+        if (desc && (type == OE_FD_TYPE_ANY || desc->type == type))
+            callback(desc, arg);
+    }
+
+    oe_spin_unlock(&_lock);
+}
diff --git a/syscall/unistd.c b/syscall/unistd.c
@@ -256,6 +256,17 @@ ssize_t oe_write(int fd, const void* buf, size_t count)
     return ret;
 }
 
+static void _close_epoll_callback(oe_fd_t* desc, void* arg)
+{
+    oe_assert(desc);
+    oe_assert(desc->type == OE_FD_TYPE_EPOLL);
+
+    const int fd = (int)(intptr_t)arg;
+    oe_assert(fd >= 0);
+
+    desc->ops.epoll.on_close(desc, fd);
+}
+
 int oe_close(int fd)
 {
     int ret = -1;
@@ -265,7 +276,13 @@ int oe_close(int fd)
         OE_RAISE_ERRNO(oe_errno);
 
     if ((ret = desc->ops.fd.close(desc)) == 0)
+    {
+        // Notify epoll instances that this fd has been closed.
+        oe_fdtable_foreach(
+            OE_FD_TYPE_EPOLL, (void*)(intptr_t)fd, _close_epoll_callback);
+
         oe_fdtable_release(fd);
+    }
 
 done:
     return ret;

diff --git a/tests/syscall/epoll/enc/enc.cpp b/tests/syscall/epoll/enc/enc.cpp
@@ -114,6 +114,36 @@ extern "C" void cancel_wait()
     _send(action_t::stop);
 }
 
+extern "C" void test_close_without_delete()
+{
+    const int epfd = epoll_create(1);
+    OE_TEST(epfd >= 0);
+
+    epoll_event event{};
+    event.events = EPOLLIN | EPOLLOUT;
+
+    // add first fd and close it
+    event.data.u32 = 1;
+    const int fd1 = socket(AF_INET, SOCK_STREAM, 0);
+    OE_TEST(fd1 >= 0);
+    OE_TEST(epoll_ctl(epfd, EPOLL_CTL_ADD, fd1, &event) == 0);
+    OE_TEST(close(fd1) == 0);
+
+    // add second fd
+    event.data.u32 = 2;
+    const int fd2 = socket(AF_INET, SOCK_STREAM, 0);
+    OE_TEST(fd2 == fd1); // expect reuse
+    OE_TEST(epoll_ctl(epfd, EPOLL_CTL_ADD, fd2, &event) == 0);
+
+    // expect epoll_wait to return the event of the second fd
+    event.data.u32 = 0;
+    OE_TEST(epoll_wait(epfd, &event, 1, 0) == 1);
+    OE_TEST(event.data.u32 == 2);
+
+    OE_TEST(close(epfd) == 0);
+    OE_TEST(close(fd2) == 0);
+}
+
 OE_SET_ENCLAVE_SGX(
     1,    /* ProductID */
     1,    /* SecurityVersion */

diff --git a/tests/syscall/epoll/epoll.edl b/tests/syscall/epoll/epoll.edl
@@ -9,5 +9,7 @@ enclave {
         public void trigger_and_add_event();
         public void trigger_and_delete_event();
         public void cancel_wait();
+
+        public void test_close_without_delete();
     };
 };
diff --git a/tests/syscall/epoll/host/host.cpp b/tests/syscall/epoll/host/host.cpp
@@ -25,6 +25,8 @@ int main(int argc, const char* argv[])
     r = oe_create_epoll_enclave(argv[1], type, flags, NULL, 0, &enclave);
     OE_TEST(r == OE_OK);
 
+    // Test concurrent use of epoll
+
     set_up(enclave);
 
     thread wait_thread(
@@ -41,6 +43,10 @@ int main(int argc, const char* argv[])
     wait_thread.join();
     tear_down(enclave);
 
+    // Test closing file descriptors without deleting them from the epoll
+    // instance
+    OE_TEST(test_close_without_delete(enclave) == OE_OK);
+
     r = oe_terminate_enclave(enclave);
     OE_TEST(r == OE_OK);