linux-kernel - Re: [PATCH 2/6] Don't assume UID 0 attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 17 Dec 2022 19:05:38 -0600
From:   evanhensbergen@...oud.com
To:     asmadeus@...ewreck.org
Cc:     v9fs-developer@...ts.sourceforge.net,
        Ron Minnich <rminnich@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux_oss@...debyte.com
Subject: Re: [PATCH 2/6] Don't assume UID 0 attach

> On Dec 17, 2022, at 6:07 PM, asmadeus@...ewreck.org wrote:
> 
> Eric Van Hensbergen wrote on Sat, Dec 17, 2022 at 06:52:06PM +0000:
>> The writeback_fid fallback code assumes a root uid fallback which
>> breaks many server configurations (which don't run as root).  This
>> patch switches to generic lookup which will follow argument
>> guidence on access modes and default ids to use on failure.
> 
> Unfortunately this one will break writes to a file created as 400 I
> think
> That's the main reason we have this writeback fid afaik -- there are
> cases where the user should be able to write to the file, but a plain
> open/write won't work... I can't think of anything else than open 400
> right now though
> 

I’ll try and craft a test case for this, but I think it works?
That being said, I haven’t been trying the xfstests, just fsx and bench.

> I'm sure there's an xfs_io command and xfstest for that, but for now:
> python3 -c 'import os; f = os.open("testfile", os.O_CREAT + os.O_RDWR, 0o400); os.write(f, b"ok\n")'
> 
> iirc ganesha running as non-root just ignores root requests and opens as
> current user-- this won't work for this particular case, but might be
> good enough for you... With that said:

Yeah, the real problem I ran into this was if the server is running as non-root this causes issues and I was testing against cpu (which doesn’t run as root).  I need to go back and check, but if you are running as root and dftuid=0 then the behavior should be the same as before?
In any case, I’ll try to go back and make this work — my big issue was always using uid 0 regardless of what mount options said is Wong.

> 
>> There is a deeper underlying problem with writeback_fids in that
>> this fallback is too standard and not an exception due to the way
>> writeback mode works in the current implementation.  Subsequent
>> patches will try to associate writeback fids from the original user
>> either by flushing on close or by holding onto fid until writeback
>> completes.
> 
> If we can address this problem though I agree we should stop using
> wrieback fids as much as we do.
> Now fids are refcounted, I think we could just use the normal fid as
> writeback fid (getting a ref), and the regular close will not clunk it
> so delayed IOs will pass.
> 
> Worth a try?

Yeah, that (using regular fids) is exactly what I am doing in my write back-fix patch which isn’t part of this series.  I was still hunting a few bugs, but I think I nailed them today.  I have to do a more extensive test sweep of the different configs, but unit tests seem good to go now so if I end up reworking the patch set to address your comment above, I may just go ahead and add it to the resubmit set.  However, I also go ahead and flush on close/clunk — and that gets rid of the delayed write back which I think is actually preferable anyways.  I may re-introduce it with temporal caching, but its just so problematic…..

         -Eric