{"thread":{"id":"62931","subject":"[Bug] Git ReadOnly Temp Packfile Causes \"Bad file descriptor\" And -13 Access Error With NFSv4","startedAt":"2025-02-10T15:57:09Z","lastAt":"2025-02-11T22:46:24Z","messageCount":4,"participants":["Maloney, Bryan","brian m. carlson","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"512178","messageId":"17DE42DF-D2A3-4E25-9198-5AF3CA516E35@amazon.com","threadId":"62931","inReplyTo":"18589D54-5E7F-434D-A890-B39102ECB293@amazon.com","subject":"[Bug] Git ReadOnly Temp Packfile Causes \"Bad file descriptor\" And -13 Access Error With NFSv4","fromName":"Maloney, Bryan","fromEmail":"bryanhm@amazon.com","sentAt":"2025-02-10T15:56:59Z","receivedAt":"2025-02-10T15:57:09Z","isPatch":false,"sender":{"key":"bryanhm@amazon.com","avatar":null},"body":"### Error\nKernel logs:\n```\nNFSv4: state recovery failed for open file pack/tmp_pack_aR0Mu3, error = -13\n```\nGit clone output:\n```\nfatal: write error: Bad file descriptor, 137.31 MiB | 45.77 MiB/s\nfatal: fetch-pack: invalid index-pack output\n```\n\n\n### Context\n\nThe following error is seen when running git clone over NFSv4 and a failover, or server restart, occurs:\n```\nNFSv4: state recovery failed for open file pack/tmp_pack_aR0Mu3, error = -13\n```\nThis error is an access denied error that happens when you try to open a file with insufficient permissions. In this case the file being opened is a read only file and it is attempted to be opened with write access.\n\nGit opens/creates this file with the O_RDWR flag but then applies read only permissions to it, 0444. Since the permissions are changed after the file is opened, the file handle works fine. However if the file was attempted to be re-opened with that same file handle we would see a -13 error. This is what we see following a failover in NFSv4. When clients reclaim their open files, the NFS server re-evaluates the file access.\n\n```\n    int mode = 0444;\n    git_path_buf(temp_filename, \"objects/%s\", pattern);\n    fd = git_mkstemp_mode(temp_filename->buf, mode);\n```\n- https://github.com/git/git/blob/master/object-file.c#L478\n\n```\n        fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n```\n- https://github.com/git/git/blob/master/wrapper.c#L491\n\nThis is an issue for active/passive HA file servers. Since NFSv4 evaluates file permissions at the time of opening a file, this FD will always get an access denied error if a failover occurs during git clone.\n\n### Reproducer\n\nOn the client, mount the NFS share with NFSv4 and run git clone using the NFS mount as the target directory.\n```\nroot@ip-10-0-85-241:/home/ubuntu# mount -t nfs -o nfsvers=4.2,nconnect=16 10.0.66.58:/share /nfsmnt\nroot@ip-10-0-85-241:/home/ubuntu# mkdir -p /nfsmnt/react\n\nroot@ip-10-0-85-241:/home/ubuntu# git clone -v https://github.com/facebook/react.git /nfsmnt/react\nCloning into '/nfsmnt/react'...\nremote: Enumerating objects: 356868, done.\nremote: Counting objects: 100% (543/543), done.\nremote: Compressing objects: 100% (256/256), done.\nfatal: write error: Bad file descriptor, 137.31 MiB | 45.77 MiB/s\nfatal: fetch-pack: invalid index-pack output\n\nroot@ip-10-0-85-241:/home/ubuntu# grep NFS /var/log/kern.log 2025-02-07T21:54:42.562725+00:00 ip-10-0-85-241 kernel: NFSv4: state recovery failed for open file pack/tmp_pack_LkbrKa, error = -13\n```\n\nOn the server, restart NFS server client is on the “Receiving objects” step of git clone.\n```\n# systemctl restart nfs-server.service \n```\n\n"},{"id":"512201","messageId":"Z6p-eIxzrFepC3py@tapette.crustytoothpaste.net","threadId":"62931","inReplyTo":"17DE42DF-D2A3-4E25-9198-5AF3CA516E35@amazon.com","subject":"Re: [Bug] Git ReadOnly Temp Packfile Causes \"Bad file descriptor\" And -13 Access Error With NFSv4","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-02-10T22:32:24Z","receivedAt":"2025-02-10T22:32:32Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-02-10 at 15:56:59, Maloney, Bryan wrote:\n> ### Error\n> Kernel logs:\n> ```\n> NFSv4: state recovery failed for open file pack/tmp_pack_aR0Mu3, error = -13\n> ```\n> Git clone output:\n> ```\n> fatal: write error: Bad file descriptor, 137.31 MiB | 45.77 MiB/s\n> fatal: fetch-pack: invalid index-pack output\n> ```\n> \n> \n> ### Context\n> \n> The following error is seen when running git clone over NFSv4 and a failover, or server restart, occurs:\n> ```\n> NFSv4: state recovery failed for open file pack/tmp_pack_aR0Mu3, error = -13\n> ```\n> This error is an access denied error that happens when you try to open a file with insufficient permissions. In this case the file being opened is a read only file and it is attempted to be opened with write access.\n> \n> Git opens/creates this file with the O_RDWR flag but then applies read only permissions to it, 0444. Since the permissions are changed after the file is opened, the file handle works fine. However if the file was attempted to be re-opened with that same file handle we would see a -13 error. This is what we see following a failover in NFSv4. When clients reclaim their open files, the NFS server re-evaluates the file access.\n\nYour description of the problem is spot on.  We intentionally set the\npermissions to 0444 because we never want anyone to change loose object\nfiles or packs, since doing so would corrupt the repository.  This\nbehaviour is specifically allowed by POSIX[0]:\n\n  The argument following the oflag argument does not affect whether the\n  file is open for reading, writing, or for both.\n\nPOSIX does not allow the re-evaluation of file system access once the\nfile is open, so it sounds like your file system is not POSIX compliant,\nand Git generally requires lots of POSIX-compliant functionality from\nthe file system. For instance, we also require the POSIX consistency\nguarantees[1], among myriad others:\n\n  If a read() of file data can be proven (by any means) to occur after a\n  write() of the data, it must reflect that write(), even if the calls\n  are made by different threads. A similar requirement applies to\n  multiple write operations to the same file position.\n\nThe implicit violation of that particular requirement is why cloud\nsyncing services often corrupt the repository.\n\nCould you adjust your NFSv4 server such that is synchronizes state among\nthe primary and replicas in case of a required failover?  I know we have\npeople successfully using Git with NFS without problems, although this\nparticular issue does often hit non-POSIX-compliant NFS implementations\nin a variety of ways.  (This particular variant is new to me, though.)\n\n> This is an issue for active/passive HA file servers. Since NFSv4 evaluates file permissions at the time of opening a file, this FD will always get an access denied error if a failover occurs during git clone.\n\nI'm not sure there's even a good way to solve this problem on the Git\nside, since I suspect that if we opened the file as 0644 and then\nimmediately did an fchmod to 0444, if you'd still fail here if the file\nis reopened.  Is that correct?\n\nI'll also point out that there's a variety of other software that does\nthe same thing as Git does, including zsh and Emacs, so fixing this in\nGit doesn't really fix the entire problem that your NFS server has,\nsince all of that other software will also be broken in at least some\ncases and require similar workarounds.  (I discovered this with a\nsimple, 30-second search on GitHub some time back.)  As far as I'm\naware, all other Git implementations also do the same thing as Git does,\nso you'd also need to patch go-git, libgit2, and every other\nimplementation as well.\n\n[0] https://pubs.opengroup.org/onlinepubs/9799919799/functions/open.html\n[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/write.html\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"512262","messageId":"20250211213940.GB3113114@coredump.intra.peff.net","threadId":"62931","inReplyTo":"Z6p-eIxzrFepC3py@tapette.crustytoothpaste.net","subject":"Re: [Bug] Git ReadOnly Temp Packfile Causes \"Bad file descriptor\" And -13 Access Error With NFSv4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-11T21:39:40Z","receivedAt":"2025-02-11T21:39:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 10, 2025 at 10:32:24PM +0000, brian m. carlson wrote:\n\n> > This is an issue for active/passive HA file servers. Since NFSv4 evaluates file permissions at the time of opening a file, this FD will always get an access denied error if a failover occurs during git clone.\n> \n> I'm not sure there's even a good way to solve this problem on the Git\n> side, since I suspect that if we opened the file as 0644 and then\n> immediately did an fchmod to 0444, if you'd still fail here if the file\n> is reopened.  Is that correct?\n\nWe could probably create and write the tempfile as 0644, and then switch\nit to 0444 before renaming it into place. We already do something\nsimilar with adjust_shared_perm(), I think.\n\nI don't know if it's worth doing it that way or not. It should just work\neven on systems that are happy with the current code, so I think the\nonly downside would be a few extra lines of code and an extra chmod()\nsyscall.\n\n-Peff\n"},{"id":"512265","messageId":"Z6vTNW9Dz4jiJvXO@tapette.crustytoothpaste.net","threadId":"62931","inReplyTo":"20250211213940.GB3113114@coredump.intra.peff.net","subject":"Re: [Bug] Git ReadOnly Temp Packfile Causes \"Bad file descriptor\" And -13 Access Error With NFSv4","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-02-11T22:46:13Z","receivedAt":"2025-02-11T22:46:24Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-02-11 at 21:39:40, Jeff King wrote:\n> We could probably create and write the tempfile as 0644, and then switch\n> it to 0444 before renaming it into place. We already do something\n> similar with adjust_shared_perm(), I think.\n> \n> I don't know if it's worth doing it that way or not. It should just work\n> even on systems that are happy with the current code, so I think the\n> only downside would be a few extra lines of code and an extra chmod()\n> syscall.\n\nThat may work here for this instance.  My concern is that we'll find\nsome other weird, hard-to-reproduce behaviour because files are silently\ngetting reopened and permissions are getting re-checked.  This bug is\nmuch worse than the standard NFS/CIFS implementation bug where open with\nO_RDWR and 0444 just fails up front, because it can happen in the middle\nof an operation we thought was going to be successful.\n\nI'm not completely opposed to a small workaround here (although, as I\nmentioned, a decent amount of other software would also need patching\nfor such a system, which argues against it), but I don't want us to have\nlots of workarounds scattered all over the code to patch one flaky set\nof NFS servers, so we'd want to limit it to this one spot, especially\nsince this won't be easy to test.\n\nI will also mention that this NFS server will also break lots of\nproprietary software.  One of my former employers had software that\nwould do lots of open/fchmod in quick succession (for good and valuable\nreasons that are too long to fit in this parenthetical), and I'm pretty\nsure that won't work properly on this server based on the described\nbehaviour.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"}]}