/proc/kcore: fix seeking

PATCH In review Dave Anderson 3 participants 7 messages Jan 10, 2011 – Jan 17, 2011

Subsystems (2)

Proc filesystem Kcore

lore

Share & tools

lore

Dave Anderson

author

2011-01-10 14:42 UTC #0 lore raw

PATCH 1/1 /proc/kcore: fix seeking

From: Dave Anderson <[email protected]>

Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.

The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.

A similar revert was made for /proc/vmcore.

Signed-off-by Dave Anderson <[email protected]>

Cc Frederic Weisbecker <[email protected]>

fs/proc/kcore.c renamed from fs/proc/kcore.c.orig +1 -1

@@ -558,7 +558,7 @@ static int open_kcore(struct inode *inod
 static const struct file_operations proc_kcore_operations = {
 	.read		= read_kcore,
 	.open		= open_kcore,
-	.llseek		= generic_file_llseek,
+	.llseek		= default_llseek,
 };
 
 #ifdef CONFIG_MEMORY_HOTPLUG

Frederic Weisbecker

Acked-by on v1 1/1

2011-01-11 00:53 UTC #1 lore raw

(Adding stable and Andrew in Cc)

Quoted 14 lines · Dave Anderson

From: Dave Anderson <[email protected]>
Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.
The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.

Quoted 4 lines

A similar revert was made for /proc/vmcore.
Signed-off-by Dave Anderson <[email protected]>
Cc Frederic Weisbecker <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

In the longer term, I guess default_llseek should disappear and replaced with generic_file_llseek(), tweaking the sb->s_maxbytes with the appropriate values in each filesystems.

Thanks for this fix!

Quoted 11 lines

fs/proc/kcore.c renamed from fs/proc/kcore.c.orig +1 -1

@@ -558,7 +558,7 @@ static int open_kcore(struct inode *inod
 static const struct file_operations proc_kcore_operations = {
 	.read		= read_kcore,
 	.open		= open_kcore,
-	.llseek		= generic_file_llseek,
+	.llseek		= default_llseek,
 };
 
 #ifdef CONFIG_MEMORY_HOTPLUG

Américo Wang

2011-01-11 16:04 UTC #2 lore raw

In reply to Dave Anderson

Quoted 8 lines · Dave Anderson

From: Dave Anderson <[email protected]>
Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.

Quoted 6 lines

The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.

Is the race solved? Using default_llseek() still races with read_kcore() on fpos, AFAIK.

Signature · 1 line

Live like a child, think like the god.

Frederic Weisbecker

2011-01-11 16:23 UTC #3 lore raw

Quoted 16 lines · Américo Wang

Quoted 8 lines · Dave Anderson
From: Dave Anderson <[email protected]>
Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.
Quoted 6 lines
The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.

Quoted 2 lines

Is the race solved? Using default_llseek() still races with read_kcore() on fpos, AFAIK.

Hmm, how does it race there?

read_kcore() manipulates fpos, which can't be changed behind us inside the read callback as it's a snapshot. Also read_kcore() can change the value of fpos, which is writed back to file->fpos from sys_read().

So the last resulting race here the natural one between seeking and reading, which is up to the user to take care of.

Or am I missing something?

Américo Wang

2011-01-14 09:45 UTC #4 lore raw

Quoted 22 lines · Frederic Weisbecker

Quoted 16 lines · Américo Wang
Quoted 8 lines · Dave Anderson
From: Dave Anderson <[email protected]>
Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.
Quoted 6 lines
The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.
Quoted 2 lines
Is the race solved? Using default_llseek() still races with read_kcore() on fpos, AFAIK.
Hmm, how does it race there?

Quoted 8 lines

read_kcore() manipulates fpos, which can't be changed behind us inside the read callback as it's a snapshot. Also read_kcore() can change the value of fpos, which is writed back to file->fpos from sys_read().
So the last resulting race here the natural one between seeking and reading, which is up to the user to take care of.

Hmm, I just read the changelog of commit 34aacb2920667d405a8df15968b7f71ba46c8f18, which claims to fix the race. So anything changed in vfs layer after that?

Thanks.

Frederic Weisbecker

2011-01-14 16:38 UTC #5 lore raw

Quoted 28 lines · Américo Wang

Quoted 27 lines · Frederic Weisbecker
Quoted 16 lines · Américo Wang
Quoted 8 lines · Dave Anderson
From: Dave Anderson <[email protected]>
Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.
Quoted 6 lines
The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.
Quoted 2 lines
Is the race solved? Using default_llseek() still races with read_kcore() on fpos, AFAIK.
Hmm, how does it race there?
read_kcore() manipulates fpos, which can't be changed behind us inside the read callback as it's a snapshot. Also read_kcore() can change the value of fpos, which is writed back to file->fpos from sys_read().

Quoted 7 lines

Quoted 3 lines
So the last resulting race here the natural one between seeking and reading, which is up to the user to take care of.
Hmm, I just read the changelog of commit 34aacb2920667d405a8df15968b7f71ba46c8f18, which claims to fix the race. So anything changed in vfs layer after that?

Ah it didn't fix any race, it just got rid of the bkl, OTOH I said in my changelog:

"/proc/kcore has no llseek and then falls down to use default_llseek. This is racy against read_kcore() that directly manipulates fpos but it doesn't hold the bkl there so using it in llseek doesn't protect anything."

So I think this just testifies my crude misunderstanding of the code when I wrote that changelog. I didn't realize fpos is a copy of the file offset that is writed back later. Hence my changelog was buggy.

Américo Wang

2011-01-17 08:06 UTC #6 lore raw

Quoted 41 lines · Frederic Weisbecker

Quoted 28 lines · Américo Wang
Quoted 27 lines · Frederic Weisbecker
Quoted 16 lines · Américo Wang
Quoted 8 lines · Dave Anderson
From: Dave Anderson <[email protected]>
Commit 34aacb2920667d405a8df15968b7f71ba46c8f18 ("procfs: Use generic_file_llseek in /proc/kcore") broke seeking on /proc/kcore. This changes it back to use default_llseek in order to restore the original behavior.
Quoted 6 lines
The problem with generic_file_llseek is that it only allows seeks up to inode->i_sb->s_maxbytes, which is 2GB-1 on procfs, where the memory file offset values in the /proc/kcore PT_LOAD segments may exceed or start beyond that offset value.
Quoted 2 lines
Is the race solved? Using default_llseek() still races with read_kcore() on fpos, AFAIK.
Hmm, how does it race there?
read_kcore() manipulates fpos, which can't be changed behind us inside the read callback as it's a snapshot. Also read_kcore() can change the value of fpos, which is writed back to file->fpos from sys_read().
Quoted 7 lines
Quoted 3 lines
So the last resulting race here the natural one between seeking and reading, which is up to the user to take care of.
Hmm, I just read the changelog of commit 34aacb2920667d405a8df15968b7f71ba46c8f18, which claims to fix the race. So anything changed in vfs layer after that?
Ah it didn't fix any race, it just got rid of the bkl, OTOH I said in my changelog:

Quoted 8 lines

"/proc/kcore has no llseek and then falls down to use default_llseek. This is racy against read_kcore() that directly manipulates fpos but it doesn't hold the bkl there so using it in llseek doesn't protect anything."
So I think this just testifies my crude misunderstanding of the code when I wrote that changelog. I didn't realize fpos is a copy of the file offset that is writed back later. Hence my changelog was buggy.

Ok, thanks for explaining this!
Dave's patch should be alright.