[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug cont
From:       Sebastian =?UTF-8?B?TMO2dmRhaGw=?= <duke () openjdk ! org>
Date:       2024-05-12 18:42:03
Message-ID: 5jtxG7mdbPDU7_iroUJP04Kut6e7o1_5XF2Ht_L9ciE=.67b51646-bcab-4fe8-a192-9d8e63a3e31b () github ! com
[Download RAW message or body]

On Mon, 6 May 2024 18:31:06 GMT, Larry Cable <duke@openjdk.org> wrote:

> > Sebastian Lövdahl has updated the pull request incrementally with one additional \
> > commit since the last revision: 
> > Reworked attach logic
> 
> On 5/6/24 10:35 AM, Sebastian Lövdahl wrote:
> > 
> > I pushed an updated attempt at this now with d3e26a0 
> > <https://urldefense.com/v3/__https://github.com/openjdk/jdk/commit/d3e26a0c444e06b \
> > a9757fd528d72d83f56cd098b__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svPHZrq9ZQ$>. \
> >  Local testing and |make test 
> > TEST="jtreg:test/hotspot/jtreg/containers"| + |make test 
> > TEST="jtreg:test/hotspot/jtreg/serviceability"| indicate that all the 
> > known use-cases work.
> > 
> > Still eager to see what you come up with @larry-cable 
> > <https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!M_ \
> > KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNMPdGFLg$>. \
> >  |createAttachFile| could still be improved for example. And I would 
> > also be interested in looking into writing some test for the elevated 
> > privileges case.
> > 
> > —
> > Reply to this email directly, view it on GitHub 
> > <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecommen \
> > t-2096564990__;Iw!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNUwHWtZA$>, \
> >  or unsubscribe 
> > <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANT \
> > A67SGOTAXCKY2TO2OBDDZA65N5AVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK \
> > 3TUHMZDAOJWGU3DIOJZGA__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svO2ymONGw$>.
> >  You are receiving this because you were mentioned.Message ID: 
> > ***@***.***>
> > 
> 
> 
> /*
> diff --git 
> a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java 
> b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> index 81d4fd259ed..c148dbd61b7 100644
> --- a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> +++ b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> @@ -34,6 +34,7 @@
> import java.nio.file.Path;
> import java.nio.file.Paths;
> import java.nio.file.Files;
> +import java.util.Optional;
> 
> import static java.nio.charset.StandardCharsets.UTF_8;
> 
> @@ -46,8 +47,28 @@ public class VirtualMachineImpl extends 
> HotSpotVirtualMachine {
> // location is the same for all processes, otherwise the tools
> // will not be able to find all Hotspot processes.
> // Any changes to this needs to be synchronized with HotSpot.
> -    private static final String tmpdir = "/tmp";
> +    private static final Path TMPD...

Thanks for all the deep thinking you're doing here @larry-cable, appreciated. And \
sorry for the delay in my response, I'll try to get more time devoted to this during \
the coming week.

> I did some thinking on this issue over the weekend and came up with an
> idea that *may* improve the probability of an attach succeeding in the
> case that the target has elevated privileges and the jcmd is not in the
> same mnt namespace as the target JVM.

I haven't fully digested the patches you have provided yet, but one question this \
far. In these cases, is it not a requirement that `jcmd` is run as `root`? So even if \
the target process is run with elevated privileges, attaching would always work. Or \
is there some way to attach from host to container with a non-`root` user that I'm \
missing?

One thing I would like to eventually achieve is to have \
[JDK-8226919](https://bugs.openjdk.org/browse/JDK-8226919) and this fix backported to \
the current LTS releases. Would it make more sense to fix \
[JDK-8327114](https://bugs.openjdk.org/browse/JDK-8327114) with as few changes as \
possible, and use that for backports, and do more extensive improvements in a \
separate follow-up that don't need backporting?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2106339738


[prev in list] [next in list] [prev in thread] [next in thread]