Re: [PATCH] fix bitmap corruption on close_range() with CLOSE_RANGE_UNSHARE

Linus Torvalds
2024-08-04T00:05:57Z

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On Sat, 3 Aug 2024 at 16:51, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> In fact, I think the full_fds_bits copy in copy_fd_bitmaps() should
> just be entirely rewritten. Even the initial copy shouldn't be done
> with some byte-wise memcpy/memset, since those are all 'unsigned long'
> arrays, and the size is aligned. So it should be done on words, but
> whatever.
>
> And the full_fds_bits case should use our actual bitmap functions.

Something ENTIRELY UNTESTED like this, IOW.

Am I just completely off my rocker? You decide.

Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2027 bytes --]

fs/file.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index a11e59b5d602..ea7f938905f3 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -46,27 +46,36 @@ static void free_fdtable_rcu(struct rcu_head *rcu)
#define BITBIT_NR(nr) BITS_TO_LONGS(BITS_TO_LONGS(nr))
#define BITBIT_SIZE(nr) (BITBIT_NR(nr) * sizeof(long))

+static inline void copy_bitmap_words(unsigned long *dst, unsigned long *src,
+ unsigned int copy, unsigned int clear)
+{
+ memcpy(dst, src, copy * sizeof(unsigned long));
+ memset(dst + copy, 0, clear * sizeof(unsigned long));
+}
+
/*
* Copy 'count' fd bits from the old table to the new table and clear the extra
* space if any. This does not copy the file pointers. Called with the files
* spinlock held for write.
+ *
+ * Note: the fd bitmaps are always full bitmap words (see sane_fdtable_size()),
+ * but the full_fds_bits bitmap is a bitmap of fd bitmap words, not of fds.
*/
static void copy_fd_bitmaps(struct fdtable *nfdt, struct fdtable *ofdt,
unsigned int count)
{
- unsigned int cpy, set;
+ unsigned int copy, clear;

- cpy = count / BITS_PER_BYTE;
- set = (nfdt->max_fds - count) / BITS_PER_BYTE;
- memcpy(nfdt->open_fds, ofdt->open_fds, cpy);
- memset((char *)nfdt->open_fds + cpy, 0, set);
- memcpy(nfdt->close_on_exec, ofdt->close_on_exec, cpy);
- memset((char *)nfdt->close_on_exec + cpy, 0, set);
+ copy = count / BITS_PER_LONG;
+ clear = (nfdt->max_fds - count) / BITS_PER_LONG;

- cpy = BITBIT_SIZE(count);
- set = BITBIT_SIZE(nfdt->max_fds) - cpy;
- memcpy(nfdt->full_fds_bits, ofdt->full_fds_bits, cpy);
- memset((char *)nfdt->full_fds_bits + cpy, 0, set);
+ /* Copy the bitmap words */
+ copy_bitmap_words(nfdt->open_fds, ofdt->open_fds, copy, clear);
+ copy_bitmap_words(nfdt->close_on_exec, ofdt->close_on_exec, copy, clear);
+
+ /* Copy the bits for the words */
+ bitmap_copy(nfdt->full_fds_bits, ofdt->full_fds_bits, copy);
+ bitmap_clear(nfdt->full_fds_bits, copy, clear);
}

/*