Re: Libfuse interoperability/ABI broken.

[Bernd Schubert <bernd.schubert@fastmail.fm>]

Hi Amir,

thanks for your help!

On 3/9/24 03:46, Amir Goldstein wrote:
> On Thu, Mar 7, 2024 at 8:47 PM Bernd Schubert
> <bernd.schubert@fastmail.fm> wrote:
>>
>> Hi all,
>>
>> this is certainly not kind of the mail I was hoping for as a new libfuse
>> maintainer.
>>
>> As you can see from the title and from discussion below (sorry this is
>> not typical ML discussion style), we have a bit of of problem with
>> libfuse ABI compatibility.
>>
>> While scanning through git history, Ashley found a commit that was adding
>> members in the middle of struct - doesn't break API, but binary
>> compatibility. That commit already landed a year ago in these releases:
>>
>> git tag --contains a5eb7f2a0117ab43119ef5724cf5f4f2f181804a
>> fuse-3.14.1
>> fuse-3.15.0
>> fuse-3.15.1
>> fuse-3.16.1
>> fuse-3.16.2
>>
>>
>> Obviously this needs improved testing for, but right now we wonder what
>> would be the preferred action to avoid issues.
>>
>> a) We could fix it and move bits to the right place. Fixes everything
>> compiled with old versions, but breaks everything compiled with the
>> versions above
>>
>> b) Increase the .so version - enforces recompilation of all binaries.
>> Intrusive, especially for distributions, but probably safest.
>>
>> c) Other ideas?
>>
> 
> Heuristically, you can detect most of the shifted flags at runtime
> because...
> 
>>
>>
>> I don't think there is anything in libfuse that would allow us to
>> detect which version of libfuse a library was linked to.
>>
> 
> I think we do know for sure if fs was linked with libfuse < 3.12
> without fuse_loop_mt_312?
> so only left to detect  3.12..3.14 vs. 3.14..3.16

Hmm, I guess I miss something, but how would I know if it was linked
with fuse_loop_mt_312? That needs an elf reader? Assuming we would put
this into the library, somehow, how does this work with stripped binaries?

bschubert2@imesrv6 example>nm passthrough_ll | head -n1
00000000000003b4 r __abi_tag
bschubert2@imesrv6 example>strip passthrough_ll
bschubert2@imesrv6 example>nm passthrough_ll | head -n1
nm: passthrough_ll: no symbols



> 
>>
>> The commit shifted these members in struct fuse_file_info {
>>
>> struct fuse_file_info {
>> ...
>>         /** Can be filled by open/create, to allow parallel direct writes on this
>>          *  file */
>>         unsigned int parallel_direct_writes : 1; --> introduced the shift
> 
> Not expected in flush/release, so can be heuristically interpreted as flush
> 
>>
>>         /** Indicates a flush operation.  Set in flush operation, also
>>             maybe set in highlevel lock operation and lowlevel release
>>             operation. */
>>         unsigned int flush : 1;
>>
> 
> Not expected in open/create, so can be heuristically interpreted as nonseekable
> 
>>         /** Can be filled in by open, to indicate that the file is not
>>             seekable. */
>>         unsigned int nonseekable : 1;
>>
> 
> Not expected in release, so can be heuristically interpreted as flock_release
> 
>>         /* Indicates that flock locks for this file should be
>>            released.  If set, lock_owner shall contain a valid value.
>>            May only be set in ->release(). */
>>         unsigned int flock_release : 1;
>>
> 
> Not expected in opendir, so can be heuristically interpreted as cache_readdir
> 
>>         /** Can be filled in by opendir. It signals the kernel to
>>             enable caching of entries returned by readdir().  Has no
>>             effect when set in other contexts (in particular it does
>>             nothing when set by open()). */
>>         unsigned int cache_readdir : 1;
>>
> 
> Ignored in open, but based on the comment above, it may be
> implied that some fs may set it in open() reply
> 
>>         /** Can be filled in by open, to indicate that flush is not needed
>>             on close. */
>>         unsigned int noflush : 1;
> 
> noflush is just an optimization, which the kernel ignores anyway
> when writeback cache is enabled, so no harm done if it is wrongly
> interpreted as readdir_cache in open() and ignored.
> It is also quite recent (3.11) so not very likely used.
> 
>> };
>>
>> I.e. setting flush would actually set parallel_direct_writes
>> (note: with binaries compiled against older libfuse versions)
>>
> 
> It would be suspicious if fs sets parallel_direct_writes flush at
> flush() or release(), so that can be used as a hint of old ABI
> interpreted as flush and issue a warning.
> 
> It would be pretty ugly to use these heuristics in the library forever,
> but perhaps as safety measure for binaries built with a range of
> libfuse version that is not likely to be found in the wild (3.14..3.16)
> it is an acceptable compromise?

I really like your idea about heuristics, I just don't know how to limit
the libfuse version range.

> 
> and perhaps the next libfuse version can pass the header version
> fs was compiled with as argument to fuse_loop_mt_317()?


I think adding it to fuse_loop_mt_317 would be easy and could be
auto-added with a macro. I think more difficult is to add it to the old
api/abi. I.e. it would take a few years until that api version gets
mostly used?