| Message ID | 20230304131528.4645d19a2ab897fb7518159e@linux-foundation.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] hotfixes for 6.3-rc1 | expand |
The pull request you sent on Sat, 4 Mar 2023 13:15:28 -0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-hotfixes-stable-2023-03-04-13-12
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/20fdfd55ab5c3fdff5b43de632a8d8fb7744e186
Thank you!
On Sat, Mar 4, 2023 at 1:15 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > 17 hotfixes. Eight are for MM and seven are for other parts of the > kernel. Seven are cc:stable and eight address post-6.3 issues or were > judged unsuitable for -stable backporting. Hmm. Since this pull didn't fix the gcc note about playing pointer games that I get for my allmodconfig test build, and since I _really_ don't want to have an rc1 release tomorrow with that (valid) warning, I fixed it up myself. I fixed up the gcc note the cleanest way I could, by using a union to make it very explicit that yes, we're basically doing a bit-for-bit assignment from one incompatible type to another. I would *not* encourage this pattern in general, but it had a comment about why that invalid pointer conversion was fine in this case, and it really does seem to be a fairly natural use of a union. This situation really is that kind of "don't convert types, just copy the bit representation". So it's kind of conceptually quite similar to the traditional "use a union to convert floating point bit representations to integers" and back (as opposed to using a cast to convert a pointer in order to then _use_ it as a pointer in the new form). See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e77d587a2c04e82c6a0dffa4a32c874a4029385d for details. At least gcc generated identical code (well, for an unholy version of that patch that had been edited to avoid line number changes) for me, so that "go through a union type" doesn't cause any other differences than getting rid of the gcc note. (And this was definitely one of the cases where I felt that the gcc note was entirely valid, and a good warning - even if it wasn't technically a warnign that would cause -Werror to trigger. So I didn't want to shut up the note by turning it off, I really wanted the code to be more clear about what it does). Linus
On Sat, 4 Mar 2023 14:35:00 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Mar 4, 2023 at 1:15 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > 17 hotfixes. Eight are for MM and seven are for other parts of the > > kernel. Seven are cc:stable and eight address post-6.3 issues or were > > judged unsuitable for -stable backporting. > > Hmm. Since this pull didn't fix the gcc note about playing pointer > games that I get for my allmodconfig test build, and since I _really_ > don't want to have an rc1 release tomorrow with that (valid) warning, > I fixed it up myself. Ah. Ying did it this way: From: Huang Ying <ying.huang@intel.com> Subject: migrate_pages: silence gcc notes for mis-casting Date: Thu, 2 Mar 2023 09:26:10 +0800 The following GCC notes was reported for commit 64c8902ed441 ("migrate_pages: split unmap_and_move() to _unmap() and _move()"). mm/migrate.c: In function `__migrate_folio_extract': mm/migrate.c:1050:20: note: randstruct: casting between randomized structure pointer types (ssa): `struct anon_vma' and `struct address_space' 1050 | *anon_vmap = (void *)dst->mapping; | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ The casting itself is safe. Because we only use dst->mapping to store the pointer itself temporarily and dst is a newly allocated folio and not used by anyone else during that. But the notes should be silenced and some comments are deserved. So, we do that in this patch. Link: https://lkml.kernel.org/r/20230302012610.17055-1-ying.huang@intel.com Fixes: 64c8902ed441 ("migrate_pages: split unmap_and_move() to _unmap() and _move()") Signed-off-by: "Huang, Ying" <ying.huang@intel.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Baolin Wang <baolin.wang@linux.alibaba.com> Cc: Xin Hao <xhao@linux.alibaba.com> Cc: Zi Yan <ziy@nvidia.com> Cc: Yang Shi <shy828301@gmail.com> Cc: Oscar Salvador <osalvador@suse.de> Cc: Matthew Wilcox <willy@infradead.org> Cc: Bharata B Rao <bharata@amd.com> Cc: Alistair Popple <apopple@nvidia.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- --- a/mm/migrate.c~migrate_pages-silence-gcc-notes-for-mis-casting +++ a/mm/migrate.c @@ -1047,7 +1047,16 @@ static void __migrate_folio_extract(stru int *page_was_mappedp, struct anon_vma **anon_vmap) { - *anon_vmap = (void *)dst->mapping; + struct anon_vma *anon_vma; + + /* + * 2 steps assignment to silence gcc notes for mis-casting. The + * casting is safe. Because we only use dst->mapping to store + * the pointer itself temporarily and dst is a newly allocated + * folio and not used by anyone else during that. + */ + anon_vma = (void *)dst->mapping; + *anon_vmap = anon_vma; *page_was_mappedp = (unsigned long)dst->private; dst->mapping = NULL; dst->private = NULL;
On Sat, Mar 4, 2023 at 3:21 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > Ah. Ying did it this way: Yeah, I saw that patch flying past, but I actually think that it only silences the warning almost by mistake. There's nothing fundamental in there that a compiler wouldn't just follow across two assignments, and it just happens to now not trigger any more. Assigning to a union entry is a more fundamental operation in that respect. Not that the compiler still doesn't see that it's assigning a value that in the end is not really type compatible, so a different version of gcc could still warn, but at that point I feel like it's more of an actual compiler bug than just "oh, the compiler didn't happen to follow the cast through a temporary". Linus
Hi, Linus, Linus Torvalds <torvalds@linux-foundation.org> writes: > On Sat, Mar 4, 2023 at 3:21 PM Andrew Morton <akpm@linux-foundation.org> wrote: >> >> Ah. Ying did it this way: > > Yeah, I saw that patch flying past, but I actually think that it only > silences the warning almost by mistake. There's nothing fundamental in > there that a compiler wouldn't just follow across two assignments, and > it just happens to now not trigger any more. > > Assigning to a union entry is a more fundamental operation in that > respect. Not that the compiler still doesn't see that it's assigning a > value that in the end is not really type compatible, so a different > version of gcc could still warn, but at that point I feel like it's > more of an actual compiler bug than just "oh, the compiler didn't > happen to follow the cast through a temporary". Yes. Your fix is much better. This can be used for __page_set_anon_rmap() family too to make the code look better? Best Regards, Huang, Ying
On 3/6/23 02:25, Huang, Ying wrote: > Hi, Linus, > > Linus Torvalds <torvalds@linux-foundation.org> writes: > >> On Sat, Mar 4, 2023 at 3:21 PM Andrew Morton <akpm@linux-foundation.org> wrote: >>> >>> Ah. Ying did it this way: >> >> Yeah, I saw that patch flying past, but I actually think that it only >> silences the warning almost by mistake. There's nothing fundamental in >> there that a compiler wouldn't just follow across two assignments, and >> it just happens to now not trigger any more. >> >> Assigning to a union entry is a more fundamental operation in that >> respect. Not that the compiler still doesn't see that it's assigning a >> value that in the end is not really type compatible, so a different >> version of gcc could still warn, but at that point I feel like it's >> more of an actual compiler bug than just "oh, the compiler didn't >> happen to follow the cast through a temporary". > > Yes. Your fix is much better. This can be used for > __page_set_anon_rmap() family too to make the code look better? Those are trickier due to the PAGE_MAPPING_ANON tagging bit which your code doesn't need to handle because you can simply store an untagged anon_vma pointer. Otherwise we could have the union at the struct page level already (but probably not at this point as IIRC Matthew is planning to have completely separate types for anon and file folios). So right now we have e.g. folio_get_anon_vma() using unsigned long as the intermediate variable, page_move_anon_rmap() using a void * variable, __page_set_anon_rmap() casting through a (void *) ... Is there a single recommended way for "tagged pointers" that we could unify that to? > Best Regards, > Huang, Ying >
Vlastimil Babka <vbabka@suse.cz> writes: > On 3/6/23 02:25, Huang, Ying wrote: >> Hi, Linus, >> >> Linus Torvalds <torvalds@linux-foundation.org> writes: >> >>> On Sat, Mar 4, 2023 at 3:21 PM Andrew Morton <akpm@linux-foundation.org> wrote: >>>> >>>> Ah. Ying did it this way: >>> >>> Yeah, I saw that patch flying past, but I actually think that it only >>> silences the warning almost by mistake. There's nothing fundamental in >>> there that a compiler wouldn't just follow across two assignments, and >>> it just happens to now not trigger any more. >>> >>> Assigning to a union entry is a more fundamental operation in that >>> respect. Not that the compiler still doesn't see that it's assigning a >>> value that in the end is not really type compatible, so a different >>> version of gcc could still warn, but at that point I feel like it's >>> more of an actual compiler bug than just "oh, the compiler didn't >>> happen to follow the cast through a temporary". >> >> Yes. Your fix is much better. This can be used for >> __page_set_anon_rmap() family too to make the code look better? > > Those are trickier due to the PAGE_MAPPING_ANON tagging bit which your > code doesn't need to handle because you can simply store an untagged > anon_vma pointer. Otherwise we could have the union at the struct page > level already (but probably not at this point as IIRC Matthew is > planning to have completely separate types for anon and file folios). > > So right now we have e.g. folio_get_anon_vma() using unsigned long as > the intermediate variable, page_move_anon_rmap() using a void * > variable, __page_set_anon_rmap() casting through a (void *) ... Is there > a single recommended way for "tagged pointers" that we could unify that to? Ah, you are right. We need to deal with tagging bit and maybe struct movable_operations *. I tried to write the below debug patch (only build test it). The code adds 1 or 2 lines for each function. But to be honest, the original force casting method appears more natural to me. Best Regards, Huang, Ying ---------------------------8<------------------------------------ From 68a0f54921beca8aeaa8fe78867e62b5a66658b8 Mon Sep 17 00:00:00 2001 From: Huang Ying <ying.huang@intel.com> Date: Thu, 9 Mar 2023 15:29:58 +0800 Subject: [PATCH] dbg mapping_ptr --- mm/rmap.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/mm/rmap.c b/mm/rmap.c index 8632e02661ac..50ee208baff9 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -466,6 +466,13 @@ void __init anon_vma_init(void) SLAB_PANIC|SLAB_ACCOUNT); } +union mapping_ptr { + struct address_space *mapping; + unsigned long tag; + struct anon_vma *anon_vma; + struct movable_operations *mops; +}; + /* * Getting a lock on a stable anon_vma from a page off the LRU is tricky! * @@ -493,16 +500,17 @@ void __init anon_vma_init(void) struct anon_vma *folio_get_anon_vma(struct folio *folio) { struct anon_vma *anon_vma = NULL; - unsigned long anon_mapping; + union mapping_ptr mptr; rcu_read_lock(); - anon_mapping = (unsigned long)READ_ONCE(folio->mapping); - if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON) + mptr.mapping = READ_ONCE(folio->mapping); + if ((mptr.tag & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON) goto out; if (!folio_mapped(folio)) goto out; - anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON); + mptr.tag &= ~PAGE_MAPPING_FLAGS; + anon_vma = mptr.anon_vma; if (!atomic_inc_not_zero(&anon_vma->refcount)) { anon_vma = NULL; goto out; @@ -1115,18 +1123,20 @@ int folio_total_mapcount(struct folio *folio) void page_move_anon_rmap(struct page *page, struct vm_area_struct *vma) { void *anon_vma = vma->anon_vma; + union mapping_ptr mptr; struct folio *folio = page_folio(page); VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio); VM_BUG_ON_VMA(!anon_vma, vma); - anon_vma += PAGE_MAPPING_ANON; + mptr.anon_vma = anon_vma; + mptr.tag |= PAGE_MAPPING_ANON; /* * Ensure that anon_vma and the PAGE_MAPPING_ANON bit are written * simultaneously, so a concurrent reader (eg folio_referenced()'s * folio_test_anon()) will not see one without the other. */ - WRITE_ONCE(folio->mapping, anon_vma); + WRITE_ONCE(folio->mapping, mptr.mapping); SetPageAnonExclusive(page); } @@ -1142,6 +1152,7 @@ static void __page_set_anon_rmap(struct folio *folio, struct page *page, struct vm_area_struct *vma, unsigned long address, int exclusive) { struct anon_vma *anon_vma = vma->anon_vma; + union mapping_ptr mptr; BUG_ON(!anon_vma); @@ -1162,8 +1173,9 @@ static void __page_set_anon_rmap(struct folio *folio, struct page *page, * the PAGE_MAPPING_ANON type identifier, otherwise the rmap code * could mistake the mapping for a struct address_space and crash. */ - anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON; - WRITE_ONCE(folio->mapping, (struct address_space *) anon_vma); + mptr.anon_vma = anon_vma; + mptr.tag |= PAGE_MAPPING_ANON; + WRITE_ONCE(folio->mapping, mptr.mapping); folio->index = linear_page_index(vma, address); out: if (exclusive)
Linus, please merge this batch of hotfixes, thanks. The following changes since commit 1ec35eadc3b448c91a6b763371a7073444e95f9d: Merge tag 'clk-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux (2023-02-25 15:16:23 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-hotfixes-stable-2023-03-04-13-12 for you to fetch changes up to ecf1d926661bec4080a79c0ac9dbfe02b31702cf: mailmap: map Dikshita Agarwal's old address to his current one (2023-03-02 21:54:24 -0800) ---------------------------------------------------------------- 17 hotfixes. Eight are for MM and seven are for other parts of the kernel. Seven are cc:stable and eight address post-6.3 issues or were judged unsuitable for -stable backporting. ---------------------------------------------------------------- Andrew Morton (1): fs/cramfs/inode.c: initialize file_ra_state Dongliang Mu (1): fs: hfsplus: fix UAF issue in hfsplus_put_super Eric Biggers (1): lib: parser: update documentation for match_NUMBER functions Guilherme G. Piccoli (1): panic: fix the panic_print NMI backtrace setting Heming Zhao via Ocfs2-devel (2): ocfs2: fix defrag path triggering jbd2 ASSERT ocfs2: fix non-auto defrag path not working issue Konrad Dybcio (3): mailmap: map Georgi Djakov's old Linaro address to his current one mailmap: map Vikash Garodia's old address to his current one mailmap: map Dikshita Agarwal's old address to his current one Marco Elver (4): kasan: emit different calls for instrumentable memintrinsics kasan: treat meminstrinsic as builtins in uninstrumented files kasan: test: fix test for new meminstrinsic instrumentation kasan, x86: don't rename memintrinsics in uninstrumented files Mikhail Zaslonko (1): lib/zlib: DFLTCC deflate does not write all available bits for Z_NO_FLUSH Naoya Horiguchi (1): mm/hwpoison: convert TTU_IGNORE_HWPOISON to TTU_HWPOISON Vlastimil Babka (1): mm/mremap: fix dup_anon_vma() in vma_merge() case 4 andrew.yang (1): mm/damon/paddr: fix missing folio_put() .mailmap | 3 +++ arch/x86/include/asm/string_64.h | 19 ----------------- fs/cramfs/inode.c | 2 +- fs/hfsplus/super.c | 4 ++-- fs/ocfs2/move_extents.c | 34 ++++++++++++------------------- include/linux/rmap.h | 2 +- kernel/panic.c | 44 ++++++++++++++++++++++++---------------- lib/Kconfig.kasan | 9 ++++++++ lib/parser.c | 14 ++++++------- lib/zlib_deflate/defutil.h | 4 +++- mm/damon/paddr.c | 7 +++---- mm/kasan/Makefile | 9 +++++++- mm/kasan/kasan.h | 4 ++++ mm/kasan/kasan_test.c | 29 ++++++++++++++++++++++++++ mm/kasan/shadow.c | 16 ++++++++++++++- mm/memory-failure.c | 8 ++++---- mm/mmap.c | 2 +- mm/rmap.c | 2 +- scripts/Makefile.kasan | 17 ++++++++++++++++ 19 files changed, 147 insertions(+), 82 deletions(-)