* Re: [PATCH v2] ipc: fix to protect IPCS lookups using RCU
2025-04-22 17:55 ` Lorenzo Stoakes
@ 2025-04-24 6:36 ` Jeongjun Park
0 siblings, 0 replies; 3+ messages in thread
From: Jeongjun Park @ 2025-04-24 6:36 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Liam.Howlett, akpm, willy, linux-kernel,
syzbot+a2b84e569d06ca3a949c
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> You sent this mail twice by mistake, and didn't link to the original
> thread, since you sent v1 2 months ago it's really useful to provide a link
> (here it's [0] ).
>
> [0]: https://lore.kernel.org/all/20250214180157.10288-1-aha310510@gmail.com/
>
> Your description is entirely missing any 'a syzkaller report encountered an
> issue whereby [blah blah blah]'. Please add this.
>
> On Tue, Apr 22, 2025 at 09:48:43PM +0900, Jeongjun Park wrote:
> > idr_for_each() is protected by rwsem, but this is not enough. If it is not
> > protected by the RCU read-critical region, when we call radix_tree_node_free()
> > via call_rcu() to free the struct radix_tree_node, the node will be freed
> > immediately, and when we read the next node in radix_tree_for_each_slot(),
> > we can read the already freed memory.
>
> What is calling call_rcu(), or radix_tree_for_each_slot() etc.? Presumably
> idr_for_each? Maybe worth saying soe xplicitly.
>
> >
> > Therefore, we need to add code to make sure that idr_for_each() is protected
> > within the RCU read-critical region when we call it in shm_destroy_orphaned().
> >
> > Reported-by: syzbot+a2b84e569d06ca3a949c@syzkaller.appspotmail.com
>
> In Matthew's reply to you in the original thread he says 'if anybody is
> wondering what the hell this is about [link]'. It would have been useful to
> include this link here [1].
>
> [1]: https://lore.kernel.org/linux-kernel/67af13f8.050a0220.21dd3.0038.GAE@google.com/
>
> > Fixes: b34a6b1da371 ("ipc: introduce shm_rmid_forced sysctl")
> > Cc: Matthew Wilcox <willy@infradead.org>
>
> You're cc-ing more people than this? There's no need for you to manually
> add Cc: lines anyway. But this is just incorrect.
>
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
>
> Matthew seemed more aware of the behaviour in this code so I'm going to
> keep my review to process nits mostly.
>
> Others can actually review the change... But it seems you are doing what
> Matthew suggested broadly.
>
> Have you run with lockdep (i.e. CONFIG_PROVE_LOCKING) enabled? What testing
> have you done?
>
> Presumably it's ok to order the rcu read critical section and rwsem locks
> this way?
To test if this patch is the correct one, I applied this patch and
CONFIG_PROVE_LOCKING and reproduced the backtrace in the syzbot report [0]
through test.c
Unfortunately, I think I need to supplement test.c more to reproduce the
KASAN crash log, but I was able to successfully call idr_for_each()
similer to the syzbot backtrace.
Fortunately, the test results showed that there were no particular
problems related to RCU, and considering that shm_destroy_orphaned() and
proc_ipc_dointvec_minmax_orphans() are only called through proc_handler,
it can be confirmed that there are no particular problems with the patch.
I'll send you the v3 patch after fixing the other things you advised.
Regards,
Jeongjun Park
[0]: https://syzkaller.appspot.com/bug?extid=a2b84e569d06ca3a949c
test.c:
```c
// gcc -o test test.c
#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <sched.h>
#include <fcntl.h>
#include <err.h>
#include <ctype.h>
#include <errno.h>
#include <time.h>
#include <inttypes.h>
#include <math.h>
#include <sys/ipc.h>
#include <sys/shm.h>
#include <sys/types.h>
#include <sys/syscall.h>
#define KEY_ID 43214321
#define SHM_SIZE 4096
#define SHM_COUNT 100
#define FAIL_IF(x) if ((x)) { \
perror(#x); \
return -1; \
}
void write_file(const char *filename, char *text) {
int fd = open(filename, O_RDWR);
write(fd, text, strlen(text));
close(fd);
}
void setup_ns(void) {
uid_t uid = getuid();
gid_t gid = getgid();
char buffer[0x100];
if (unshare(CLONE_NEWUSER | CLONE_NEWNS | CLONE_NEWIPC)) {
perror(" [-] unshare(CLONE_NEWUSER | CLONE_NEWNS | CLONE_NEWIPC)");
exit(EXIT_FAILURE);
}
write_file("/proc/self/setgroups", "deny");
snprintf(buffer, sizeof(buffer), "0 %d 1", uid);
write_file("/proc/self/uid_map", buffer);
snprintf(buffer, sizeof(buffer), "0 %d 1", gid);
write_file("/proc/self/gid_map", buffer);
}
int main() {
setup_ns();
int key_id, shm_id;
for (int i = 0; i < SHM_COUNT; i++) {
key_id = i + KEY_ID;
shm_id = shmget((key_t)key_id, SHM_SIZE, IPC_CREAT | 0666);
if (shm_id == -1)
FAIL_IF("main: shmget error\n");
void *shmaddr = shmat(shm_id, (void*)0, 0);
if (shmaddr == -1)
FAIL_IF("main: shmat error\n");
int fd = open("/proc/sys/kernel/shm_rmid_forced", O_RDWR);
if (fd < 0)
FAIL_IF("open /proc/sys/kernel/shm_rmid_forced error\n");
write(fd, "1", 1);
close(fd);
}
return 0;
}
```
>
> > ---
> > v2: Change description and coding style
>
> Actually you completely change what you're doing here, this is a completely
> inaccurate changelog, before you dropped the rwsem, now you don't!! You
> should note this (i.e. - what it is you're actually doing...)
>
> Not sure what on earth you are changing in the coding style? You're adding
> braces and 2 lines of code?
>
> I don't think you really need to note a change in description, that's
> implicit.
>
> Also a:
>
> v1:
> https://lore.kernel.org/all/20250214180157.10288-1-aha310510@gmail.com/
>
> Is vital esp. after such a delay, as noted above.
>
> > ---
> > ipc/shm.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 99564c870084..492fcc699985 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -431,8 +431,11 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
> > void shm_destroy_orphaned(struct ipc_namespace *ns)
> > {
> > down_write(&shm_ids(ns).rwsem);
> > - if (shm_ids(ns).in_use)
> > + if (shm_ids(ns).in_use) {
> > + rcu_read_lock();
> > idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
> > + rcu_read_unlock();
> > + }
> > up_write(&shm_ids(ns).rwsem);
> > }
> >
> > --
^ permalink raw reply [flat|nested] 3+ messages in thread