Merge pull request #5264 from rnavarro/fix/vti-updown-db-flock-race

ipsec: T8975: lock vti-up-down state DB to prevent lost updates under concurrent rekey

2 files changed, 47 insertions, 35 deletions

diff --git a/python/vyos/ifconfig/vti.py b/python/vyos/ifconfig/vti.py
index 030aa1ed7..01d724593 100644
--- a/python/vyos/ifconfig/vti.py
+++ b/python/vyos/ifconfig/vti.py
@@ -15,7 +15,7 @@
 
 from vyos.ifconfig.interface import Interface
 from vyos.utils.dict import dict_search
-from vyos.utils.vti_updown_db import vti_updown_db_exists, open_vti_updown_db_readonly
+from vyos.utils.vti_updown_db import open_vti_updown_db_readonly
 
 @Interface.register
 class VTIIf(Interface):
@@ -64,14 +64,14 @@ class VTIIf(Interface):
         """
         Set interface administrative state to be 'up' or 'down'.
 
-        The interface will only be brought 'up' if ith is attached to an
+        The interface will only be brought 'up' if it is attached to an
         active ipsec site-to-site connection or remote access connection.
         """
         if state == 'down' or self.bypass_vti_updown_db:
             super().set_admin_state(state)
-        elif vti_updown_db_exists():
+        else:
             with open_vti_updown_db_readonly() as db:
-                if db.wantsInterfaceUp(self.ifname):
+                if db is not None and db.wantsInterfaceUp(self.ifname):
                     super().set_admin_state(state)
 
     def get_mac(self):
diff --git a/python/vyos/utils/vti_updown_db.py b/python/vyos/utils/vti_updown_db.py
index 2931df86c..390db5195 100644
--- a/python/vyos/utils/vti_updown_db.py
+++ b/python/vyos/utils/vti_updown_db.py
@@ -18,55 +18,67 @@ import os
 from contextlib import contextmanager
 from syslog import syslog
 
+from vyos.utils.locking import Lock
+
 VTI_WANT_UP_IFLIST = '/tmp/ipsec_vti_interfaces'
+VTI_UPDOWN_LOCK_NAME = 'ipsec_vti_updown'
 
 def vti_updown_db_exists():
     """ Returns true if the database exists """
     return os.path.exists(VTI_WANT_UP_IFLIST)
 
 @contextmanager
-def open_vti_updown_db_for_create_or_update():
-    """ Opens the database for reading and writing, creating the database if it does not exist """
-    if vti_updown_db_exists():
-        f = open(VTI_WANT_UP_IFLIST, 'r+')
-    else:
-        f = open(VTI_WANT_UP_IFLIST, 'x+')
+def _vti_updown_db_lock():
+    """Serialise access to the VTI up/down DB across the concurrent updown-hook
+    invocations (one per VTI) that strongSwan fires during a coordinated rekey,
+    which would otherwise lost-update the shared state file."""
+    lock = Lock(VTI_UPDOWN_LOCK_NAME)
+    lock.acquire()  # timeout=0 -> block until acquired
     try:
-        db = VTIUpDownDB(f)
-        yield db
+        yield
     finally:
-        f.close()
+        lock.release()
+
+
+@contextmanager
+def open_vti_updown_db_for_create_or_update():
+    """ Opens the database for reading and writing, creating the database if it does not exist """
+    with _vti_updown_db_lock():
+        mode = 'r+' if vti_updown_db_exists() else 'x+'
+        with open(VTI_WANT_UP_IFLIST, mode) as f:
+            yield VTIUpDownDB(f)
 
 @contextmanager
 def open_vti_updown_db_for_update():
     """ Opens the database for reading and writing, returning an error if it does not exist """
-    f = open(VTI_WANT_UP_IFLIST, 'r+')
-    try:
-        db = VTIUpDownDB(f)
-        yield db
-    finally:
-        f.close()
+    with _vti_updown_db_lock():
+        with open(VTI_WANT_UP_IFLIST, 'r+') as f:
+            yield VTIUpDownDB(f)
 
 @contextmanager
 def open_vti_updown_db_readonly():
-    """ Opens the database for reading, returning an error if it does not exist """
-    f = open(VTI_WANT_UP_IFLIST, 'r')
-    try:
-        db = VTIUpDownDB(f)
-        yield db
-    finally:
-        f.close()
+    """Opens the database for reading. Yields None if the database does not exist."""
+    with _vti_updown_db_lock():
+        if not vti_updown_db_exists():
+            yield None
+            return
+        with open(VTI_WANT_UP_IFLIST, 'r') as f:
+            yield VTIUpDownDB(f)
 
 def remove_vti_updown_db():
-    """ Brings down any interfaces referenced by the database and removes the database """
-    # We need to process the DB first to bring down any interfaces still up
-    with open_vti_updown_db_for_update() as db:
-        db.removeAllOtherInterfaces([])
-        # this usage of commit will only ever bring down interfaces,
-        # do not need to provide a functional interface dict supplier
-        db.commit(lambda _: None)
-
-    os.unlink(VTI_WANT_UP_IFLIST)
+    """Brings down any interfaces referenced by the database and removes the database, if it exists."""
+    with _vti_updown_db_lock():
+        if not vti_updown_db_exists():
+            return
+        # We hold the lock already; open the file directly rather than via the
+        # locking context manager to avoid re-acquiring (which would deadlock).
+        with open(VTI_WANT_UP_IFLIST, 'r+') as f:
+            db = VTIUpDownDB(f)
+            db.removeAllOtherInterfaces([])
+            # this usage of commit will only ever bring down interfaces,
+            # do not need to provide a functional interface dict supplier
+            db.commit(lambda _: None)
+        os.unlink(VTI_WANT_UP_IFLIST)
 
 class VTIUpDownDB:
     # The VTI Up-Down DB is a text-based database of space-separated "ifspecs".