From 75eabfeb7cb315c33d0f23327b510ffc59213d57 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Fri, 26 Apr 2019 01:38:04 +0000 Subject: [PATCH] Revert "Add NATT keepalive resources and methods into IpSecService" This reverts commit 4aac3e9e4896871c64b0fec127c9bafaede9646c. Reason for revert: Adds dependency between IpSecService and ConnectivityService may lead to future deadlock problems. Uses a simpler approach instead, hence the solution is not needed. See aosp/954040. Change-Id: Ibff278a6eee666cd85dba81c2bed94d568679b02 --- core/java/android/net/IIpSecService.aidl | 4 - .../java/com/android/server/IpSecService.java | 163 ++---------------- 2 files changed, 10 insertions(+), 157 deletions(-) diff --git a/core/java/android/net/IIpSecService.aidl b/core/java/android/net/IIpSecService.aidl index b5a2a16c7f..d6774d47b4 100644 --- a/core/java/android/net/IIpSecService.aidl +++ b/core/java/android/net/IIpSecService.aidl @@ -72,8 +72,4 @@ interface IIpSecService int tunnelResourceId, int direction, int transformResourceId, in String callingPackage); void removeTransportModeTransforms(in ParcelFileDescriptor socket); - - int lockEncapSocketForNattKeepalive(int encapSocketResourceId, int requesterUid); - - void releaseNattKeepalive(int nattKeepaliveResourceId, int ownerUid); } diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java index c1f52552ea..b4e1c32f25 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -28,7 +28,6 @@ import static android.system.OsConstants.SOCK_DGRAM; import static com.android.internal.util.Preconditions.checkNotNull; import android.annotation.NonNull; -import android.annotation.Nullable; import android.app.AppOpsManager; import android.content.Context; import android.content.pm.PackageManager; @@ -44,7 +43,6 @@ import android.net.IpSecTunnelInterfaceResponse; import android.net.IpSecUdpEncapResponse; import android.net.LinkAddress; import android.net.Network; -import android.net.NetworkStack; import android.net.NetworkUtils; import android.net.TrafficStats; import android.net.util.NetdService; @@ -77,8 +75,6 @@ import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.UnknownHostException; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; import java.util.List; /** @@ -179,14 +175,6 @@ public class IpSecService extends IIpSecService.Stub { void freeUnderlyingResources() throws RemoteException; } - /** - * Sentinel value placeholder for a real binder in RefcountedResources. - * - *

Used for cases where there the allocating party is a system service, and thus is expected - * to track the resource lifecycles instead of IpSecService. - */ - private static final Binder DUMMY_BINDER = new Binder(); - /** * RefcountedResource manages references and dependencies in an exclusively acyclic graph. * @@ -201,42 +189,24 @@ public class IpSecService extends IIpSecService.Stub { */ @VisibleForTesting public class RefcountedResource implements IBinder.DeathRecipient { - - @NonNull private final T mResource; - @NonNull private final List mChildren; + private final T mResource; + private final List mChildren; int mRefCount = 1; // starts at 1 for user's reference. + IBinder mBinder; - /* - * This field can take one of three states: - * 1. null, when the object has been released by the user (or the user's binder dies) - * 2. DUMMY_BINDER, when the refcounted resource is allocated from another system service - * and the other system service manages the lifecycle of the resource instead of - * IpSecService. - * 3. an actual binder, to ensure IpSecService can cleanup after users that forget to close - * their resources. - */ - @Nullable IBinder mBinder; - - RefcountedResource(@NonNull T resource, @NonNull RefcountedResource... children) { - this(resource, DUMMY_BINDER, children); - } - - RefcountedResource( - @NonNull T resource, - @NonNull IBinder binder, - @NonNull RefcountedResource... children) { + RefcountedResource(T resource, IBinder binder, RefcountedResource... children) { synchronized (IpSecService.this) { this.mResource = resource; + this.mChildren = new ArrayList<>(children.length); this.mBinder = binder; - this.mChildren = Collections.unmodifiableList(Arrays.asList(children)); + for (RefcountedResource child : children) { + mChildren.add(child); child.mRefCount++; } try { - if (mBinder != DUMMY_BINDER) { - mBinder.linkToDeath(this, 0); - } + mBinder.linkToDeath(this, 0); } catch (RemoteException e) { binderDied(); e.rethrowFromSystemServer(); @@ -283,12 +253,11 @@ public class IpSecService extends IIpSecService.Stub { return; } - if (mBinder != DUMMY_BINDER) { - mBinder.unlinkToDeath(this, 0); - } + mBinder.unlinkToDeath(this, 0); mBinder = null; mResource.invalidate(); + releaseReference(); } @@ -413,8 +382,6 @@ public class IpSecService extends IIpSecService.Stub { new RefcountedResourceArray<>(EncapSocketRecord.class.getSimpleName()); final RefcountedResourceArray mTunnelInterfaceRecords = new RefcountedResourceArray<>(TunnelInterfaceRecord.class.getSimpleName()); - final RefcountedResourceArray mNattKeepaliveRecords = - new RefcountedResourceArray<>(NattKeepaliveRecord.class.getSimpleName()); /** * Trackers for quotas for each of the OwnedResource types. @@ -428,8 +395,6 @@ public class IpSecService extends IIpSecService.Stub { final ResourceTracker mSpiQuotaTracker = new ResourceTracker(MAX_NUM_SPIS); final ResourceTracker mTransformQuotaTracker = new ResourceTracker(MAX_NUM_TRANSFORMS); final ResourceTracker mSocketQuotaTracker = new ResourceTracker(MAX_NUM_ENCAP_SOCKETS); - final ResourceTracker mNattKeepaliveQuotaTracker = - new ResourceTracker(MAX_NUM_ENCAP_SOCKETS); // Max 1 NATT keepalive per encap socket final ResourceTracker mTunnelQuotaTracker = new ResourceTracker(MAX_NUM_TUNNEL_INTERFACES); void removeSpiRecord(int resourceId) { @@ -448,10 +413,6 @@ public class IpSecService extends IIpSecService.Stub { mEncapSocketRecords.remove(resourceId); } - void removeNattKeepaliveRecord(int resourceId) { - mNattKeepaliveRecords.remove(resourceId); - } - @Override public String toString() { return new StringBuilder() @@ -461,8 +422,6 @@ public class IpSecService extends IIpSecService.Stub { .append(mTransformQuotaTracker) .append(", mSocketQuotaTracker=") .append(mSocketQuotaTracker) - .append(", mNattKeepaliveQuotaTracker=") - .append(mNattKeepaliveQuotaTracker) .append(", mTunnelQuotaTracker=") .append(mTunnelQuotaTracker) .append(", mSpiRecords=") @@ -471,8 +430,6 @@ public class IpSecService extends IIpSecService.Stub { .append(mTransformRecords) .append(", mEncapSocketRecords=") .append(mEncapSocketRecords) - .append(", mNattKeepaliveRecords=") - .append(mNattKeepaliveRecords) .append(", mTunnelInterfaceRecords=") .append(mTunnelInterfaceRecords) .append("}") @@ -617,11 +574,6 @@ public class IpSecService extends IIpSecService.Stub { mArray.remove(key); } - @VisibleForTesting - int size() { - return mArray.size(); - } - @Override public String toString() { return mArray.toString(); @@ -1032,50 +984,6 @@ public class IpSecService extends IIpSecService.Stub { } } - /** - * Tracks a NATT-keepalive instance - * - *

This class ensures that while a NATT-keepalive is active, the UDP encap socket that it is - * supporting will stay open until the NATT-keepalive is finished. NATT-keepalive offload - * lifecycles will be managed by ConnectivityService, which will validate that the UDP Encap - * socket is owned by the requester, and take a reference to it via this NattKeepaliveRecord - * - *

It shall be the responsibility of the caller to ensure that instances of an EncapSocket do - * not spawn multiple instances of NATT keepalives (and thereby register duplicate records) - */ - private final class NattKeepaliveRecord extends OwnedResourceRecord { - NattKeepaliveRecord(int resourceId) { - super(resourceId); - } - - @Override - @GuardedBy("IpSecService.this") - public void freeUnderlyingResources() { - Log.d(TAG, "Natt Keepalive released: " + mResourceId); - - getResourceTracker().give(); - } - - @Override - protected ResourceTracker getResourceTracker() { - return getUserRecord().mNattKeepaliveQuotaTracker; - } - - @Override - public void invalidate() { - getUserRecord().removeNattKeepaliveRecord(mResourceId); - } - - @Override - public String toString() { - return new StringBuilder() - .append("{super=") - .append(super.toString()) - .append("}") - .toString(); - } - } - /** * Constructs a new IpSecService instance * @@ -1911,57 +1819,6 @@ public class IpSecService extends IIpSecService.Stub { } } - private void verifyNetworkStackCaller() { - if (mContext.checkCallingOrSelfPermission(NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK) - != PackageManager.PERMISSION_GRANTED - && mContext.checkCallingOrSelfPermission(android.Manifest.permission.NETWORK_STACK) - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException( - "Requires permission NETWORK_STACK or MAINLINE_NETWORK_STACK"); - } - } - - /** - * Validates that a provided UID owns the encapSocket, and creates a NATT keepalive record - * - *

For system server use only. Caller must have NETWORK_STACK permission - * - * @param encapSocketResourceId resource identifier of the encap socket record - * @param ownerUid the UID of the caller. Used to verify ownership. - * @return - */ - public synchronized int lockEncapSocketForNattKeepalive( - int encapSocketResourceId, int ownerUid) { - verifyNetworkStackCaller(); - - // Verify ownership. Will throw IllegalArgumentException if the UID specified does not - // own the specified UDP encapsulation socket - UserRecord userRecord = mUserResourceTracker.getUserRecord(ownerUid); - RefcountedResource refcountedSocketRecord = - userRecord.mEncapSocketRecords.getRefcountedResourceOrThrow(encapSocketResourceId); - - // Build NattKeepaliveRecord - final int resourceId = mNextResourceId++; - userRecord.mNattKeepaliveRecords.put( - resourceId, - new RefcountedResource( - new NattKeepaliveRecord(resourceId), refcountedSocketRecord)); - - return resourceId; - } - - /** - * Release a previously allocated NattKeepalive that has been registered with the system server - */ - @Override - public synchronized void releaseNattKeepalive(int nattKeepaliveResourceId, int ownerUid) - throws RemoteException { - verifyNetworkStackCaller(); - - UserRecord userRecord = mUserResourceTracker.getUserRecord(ownerUid); - releaseResource(userRecord.mNattKeepaliveRecords, nattKeepaliveResourceId); - } - @Override protected synchronized void dump(FileDescriptor fd, PrintWriter pw, String[] args) { mContext.enforceCallingOrSelfPermission(DUMP, TAG);