* [patch 11/11] btrfs: The file argument for fsync() is never null
@ 2010-05-29 9:49 Dan Carpenter
2010-06-14 20:07 ` Johannes Hirte
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2010-05-29 9:49 UTC (permalink / raw)
To: linux-btrfs
Cc: Yan Zheng, Josef Bacik, Christoph Hellwig, Chris Mason, kernel-janitors
The "file" argument for fsync is never null so we can remove this check.
What drew my attention here is that 7ea8085910e: "drop unused dentry
argument to ->fsync" introduced an unconditional dereference at the
start of the function and that generated a smatch warning.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 787b50a..e252d23 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1140,7 +1140,7 @@ int btrfs_sync_file(struct file *file, int datasync)
/*
* ok we haven't committed the transaction yet, lets do a commit
*/
- if (file && file->private_data)
+ if (file->private_data)
btrfs_ioctl_trans_end(file);
trans = btrfs_start_transaction(root, 0);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-05-29 9:49 [patch 11/11] btrfs: The file argument for fsync() is never null Dan Carpenter
@ 2010-06-14 20:07 ` Johannes Hirte
2010-06-14 20:49 ` Dan Carpenter
2010-06-14 20:58 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Johannes Hirte @ 2010-06-14 20:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-btrfs, Yan Zheng, Josef Bacik, Christoph Hellwig,
Chris Mason, kernel-janitors
Am Samstag 29 Mai 2010, 11:49:07 schrieb Dan Carpenter:
> The "file" argument for fsync is never null so we can remove this check.
>
> What drew my attention here is that 7ea8085910e: "drop unused dentry
> argument to ->fsync" introduced an unconditional dereference at the
> start of the function and that generated a smatch warning.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 787b50a..e252d23 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1140,7 +1140,7 @@ int btrfs_sync_file(struct file *file, int datasync)
> /*
> * ok we haven't committed the transaction yet, lets do a commit
> */
> - if (file && file->private_data)
> + if (file->private_data)
> btrfs_ioctl_trans_end(file);
>
> trans = btrfs_start_transaction(root, 0);
I think you're wrong here. I've run into a kernel null pointer dereference at
this point with a NFS exported btrfs filesystem:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000098
IP: [<ffffffff8115d7b3>] btrfs_sync_file+0xa7/0x15a
PGD 2a72e067 PUD 1c29f067 PMD 0
Oops: 0000 [#1] SMP
last sysfs file: /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/power_method
CPU 1
Modules linked in: snd_seq_midi snd_emu10k1_synth snd_emux_synth
snd_seq_virmidi snd_seq_midi_emul snd_seq_oss snd_seq_midi_event snd_seq
snd_pcm_oss snd_mixer_oss radeon ttm drm_kms_helper drm i2c_algo_bit
snd_emu10k1 snd_rawmidi snd_ac97_codec ac97_bus snd_pcm snd_seq_device
snd_timer snd_page_alloc snd_util_mem snd_hwdep snd amd64_edac_mod edac_core
uhci_hcd ohci_hcd sata_sil edac_mce_amd sg sr_mod k8temp i2c_amd756
i2c_amd8111 hwmon
Pid: 2330, comm: nfsd Not tainted 2.6.34-btrfs-2-drm #1 TYAN Tiger K8W Dual
AMD Opteron, S2875/To Be Filled By O.E.M.
RIP: 0010:[<ffffffff8115d7b3>] [<ffffffff8115d7b3>] btrfs_sync_file+0xa7/0x15a
RSP: 0000:ffff880119e9bcf0 EFLAGS: 00010202
RAX: 0000000000000000 RBX: ffff88011e4da000 RCX: 0000000000000020
RDX: 0000000000000df8 RSI: 0000000000000000 RDI: ffff88011e495b28
RBP: ffff88011c30eb78 R08: ffffffff8141bab0 R09: 0000000000000000
R10: ffff880119e9bc50 R11: ffff88011c30eb78 R12: ffff88011c305240
R13: 0000000000000000 R14: ffffffff8141bab0 R15: ffff880119d1d040
FS: 00002ac4c0279180(0000) GS:ffff880001880000(0000) knlGS:00000000f74726d0
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000098 CR3: 0000000015c84000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process nfsd (pid: 2330, threadinfo ffff880119e9a000, task ffff880119e185e0)
Stack:
ffff8800273da3d8 0000000200000000 0000000000000003 0000000000000000
<0> ffff88011c305240 0000000000000000 ffff88011c30ec90 ffffffff810b1a99
<0> ffff880119d1d040 ffff880000000000 0000000000000000 ffff880023e63240
Call Trace:
[<ffffffff810b1a99>] ? vfs_fsync_range+0x7a/0xa7
[<ffffffff8112e055>] ? nfsd4_create_clid_dir+0x12d/0x15d
[<ffffffff81129e64>] ? nfsd4_open_confirm+0xec/0x118
[<ffffffff8111dcb1>] ? nfsd4_proc_compound+0x1fd/0x3b5
[<ffffffff8110f1cb>] ? nfsd_dispatch+0xdf/0x1b5
[<ffffffff8132e89a>] ? svc_process+0x41d/0x703
[<ffffffff8102bcc3>] ? default_wake_function+0x0/0xf
[<ffffffff8110f6dd>] ? nfsd+0xe1/0x125
[<ffffffff8110f5fc>] ? nfsd+0x0/0x125
[<ffffffff81041fcd>] ? kthread+0x75/0x7d
[<ffffffff81002b54>] ? kernel_thread_helper+0x4/0x10
[<ffffffff81041f58>] ? kthread+0x0/0x7d
[<ffffffff81002b50>] ? kernel_thread_helper+0x0/0x10
Code: 8b bb 28 01 00 00 89 44 24 08 48 81 c7 28 1b 00 00 e8 1e 79 1f 00 8b 44
24 08 e9 b4 00 00 00 48 81 c7 28 1b 00 00 e8 09 79 1f 00 <49> 83 bd 98 00 00
00 00 74 08 4c 89 ef e8 06 75 01 00 31 f6 48
RIP [<ffffffff8115d7b3>] btrfs_sync_file+0xa7/0x15a
RSP <ffff880119e9bcf0>
CR2: 0000000000000098
---[ end trace 5c7989eaf4eda923 ]---
addr2line showed me exact this change you made.
It happend with a linux-2.6.34 with the latest btrfs-changes on top.
regards,
Johannes
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-14 20:07 ` Johannes Hirte
@ 2010-06-14 20:49 ` Dan Carpenter
2010-06-14 20:58 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2010-06-14 20:49 UTC (permalink / raw)
To: Johannes Hirte
Cc: linux-btrfs, Yan Zheng, Josef Bacik, Christoph Hellwig,
Chris Mason, kernel-janitors
>
> addr2line showed me exact this change you made.
>
> It happend with a linux-2.6.34 with the latest btrfs-changes on top.
>
Yes. It used to be possible in 2.6.34 to get have file be null but in
2.6.35 it's not. Did you pull in 7ea8085910e: "drop unused dentry
argument to ->fsync"? That introduces a dereference at the start of the
function.
Sorry about this. I didn't take back porting into consideration.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-14 20:07 ` Johannes Hirte
2010-06-14 20:49 ` Dan Carpenter
@ 2010-06-14 20:58 ` Christoph Hellwig
2010-06-14 21:11 ` Dan Carpenter
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2010-06-14 20:58 UTC (permalink / raw)
To: Johannes Hirte
Cc: Dan Carpenter, linux-btrfs, Yan Zheng, Josef Bacik,
Christoph Hellwig, Chris Mason, kernel-janitors
On Mon, Jun 14, 2010 at 10:07:23PM +0200, Johannes Hirte wrote:
> I think you're wrong here. I've run into a kernel null pointer dereference at
> this point with a NFS exported btrfs filesystem:
Looks like you've applied the patch to a far too old kernel. It can't
be NULL for quite a while already.
If you're testing patches posted to the mailinglists always make sure
you have a recent enough kernel.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-14 20:58 ` Christoph Hellwig
@ 2010-06-14 21:11 ` Dan Carpenter
2010-06-14 21:16 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2010-06-14 21:11 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Johannes Hirte, linux-btrfs, Yan Zheng, Josef Bacik, Chris Mason,
kernel-janitors
On Mon, Jun 14, 2010 at 10:58:49PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 14, 2010 at 10:07:23PM +0200, Johannes Hirte wrote:
> > I think you're wrong here. I've run into a kernel null pointer dereference at
> > this point with a NFS exported btrfs filesystem:
>
> Looks like you've applied the patch to a far too old kernel. It can't
> be NULL for quite a while already.
>
You're the expert, but it looks like it could be null in 2.6.34 like he
says. I'm just looking at vfs_fsync_range() in
"git show v2.6.34:fs/sync.c".
> If you're testing patches posted to the mailinglists always make sure
> you have a recent enough kernel.
This actually was merged into the btrfs and the kernel.org trees
already.
I'm not sure how the btrfs backports work...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-14 21:11 ` Dan Carpenter
@ 2010-06-14 21:16 ` Christoph Hellwig
2010-06-14 21:45 ` Johannes Hirte
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2010-06-14 21:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Christoph Hellwig, Johannes Hirte, linux-btrfs, Yan Zheng,
Josef Bacik, Chris Mason, kernel-janitors
On Mon, Jun 14, 2010 at 11:11:20PM +0200, Dan Carpenter wrote:
> > Looks like you've applied the patch to a far too old kernel. It can't
> > be NULL for quite a while already.
> >
>
> You're the expert, but it looks like it could be null in 2.6.34 like he
> says. I'm just looking at vfs_fsync_range() in
> "git show v2.6.34:fs/sync.c".
2.6.34 is far too old.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-14 21:16 ` Christoph Hellwig
@ 2010-06-14 21:45 ` Johannes Hirte
2010-06-15 0:08 ` Chris Mason
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Hirte @ 2010-06-14 21:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Dan Carpenter, linux-btrfs, Yan Zheng, Josef Bacik, Chris Mason,
kernel-janitors
Am Montag 14 Juni 2010, 23:16:01 schrieb Christoph Hellwig:
> On Mon, Jun 14, 2010 at 11:11:20PM +0200, Dan Carpenter wrote:
> > > Looks like you've applied the patch to a far too old kernel. It can't
> > > be NULL for quite a while already.
> >
> > You're the expert, but it looks like it could be null in 2.6.34 like he
> > says. I'm just looking at vfs_fsync_range() in
> > "git show v2.6.34:fs/sync.c".
>
> 2.6.34 is far too old.
For the changes yes, but not for working. I needed the btrfs fixes without all
the other bugs introduced with 2.6.35-rc. I was to careless and pulled to much
changes in. My fault.
regards,
Johannes
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-14 21:45 ` Johannes Hirte
@ 2010-06-15 0:08 ` Chris Mason
2010-06-16 18:04 ` Johannes Hirte
0 siblings, 1 reply; 9+ messages in thread
From: Chris Mason @ 2010-06-15 0:08 UTC (permalink / raw)
To: Johannes Hirte
Cc: Christoph Hellwig, Dan Carpenter, linux-btrfs, Yan Zheng,
Josef Bacik, kernel-janitors
On Mon, Jun 14, 2010 at 11:45:40PM +0200, Johannes Hirte wrote:
> Am Montag 14 Juni 2010, 23:16:01 schrieb Christoph Hellwig:
> > On Mon, Jun 14, 2010 at 11:11:20PM +0200, Dan Carpenter wrote:
> > > > Looks like you've applied the patch to a far too old kernel. It can't
> > > > be NULL for quite a while already.
> > >
> > > You're the expert, but it looks like it could be null in 2.6.34 like he
> > > says. I'm just looking at vfs_fsync_range() in
> > > "git show v2.6.34:fs/sync.c".
> >
> > 2.6.34 is far too old.
>
> For the changes yes, but not for working. I needed the btrfs fixes without all
> the other bugs introduced with 2.6.35-rc. I was to careless and pulled to much
> changes in. My fault.
Well, my fault. I usually keep the btrfs-unstable tree against one
release old, and the users have come to expect it.
I'll make a .34 branch that works.
-chris
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch 11/11] btrfs: The file argument for fsync() is never null
2010-06-15 0:08 ` Chris Mason
@ 2010-06-16 18:04 ` Johannes Hirte
0 siblings, 0 replies; 9+ messages in thread
From: Johannes Hirte @ 2010-06-16 18:04 UTC (permalink / raw)
To: Chris Mason
Cc: Christoph Hellwig, Dan Carpenter, linux-btrfs, Yan Zheng,
Josef Bacik, kernel-janitors
Am Dienstag 15 Juni 2010, 02:08:20 schrieb Chris Mason:
> On Mon, Jun 14, 2010 at 11:45:40PM +0200, Johannes Hirte wrote:
> > Am Montag 14 Juni 2010, 23:16:01 schrieb Christoph Hellwig:
> > > On Mon, Jun 14, 2010 at 11:11:20PM +0200, Dan Carpenter wrote:
> > > > > Looks like you've applied the patch to a far too old kernel. It
> > > > > can't be NULL for quite a while already.
> > > >
> > > > You're the expert, but it looks like it could be null in 2.6.34 like
> > > > he says. I'm just looking at vfs_fsync_range() in
> > > > "git show v2.6.34:fs/sync.c".
> > >
> > > 2.6.34 is far too old.
> >
> > For the changes yes, but not for working. I needed the btrfs fixes
> > without all the other bugs introduced with 2.6.35-rc. I was to careless
> > and pulled to much changes in. My fault.
>
> Well, my fault. I usually keep the btrfs-unstable tree against one
> release old, and the users have come to expect it.
>
> I'll make a .34 branch that works.
>
> -chris
What about backporting only the important patches to the stable series? Or
would this be to much work for a still experimental filesystem?
regards,
Johannes
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-06-16 18:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-29 9:49 [patch 11/11] btrfs: The file argument for fsync() is never null Dan Carpenter
2010-06-14 20:07 ` Johannes Hirte
2010-06-14 20:49 ` Dan Carpenter
2010-06-14 20:58 ` Christoph Hellwig
2010-06-14 21:11 ` Dan Carpenter
2010-06-14 21:16 ` Christoph Hellwig
2010-06-14 21:45 ` Johannes Hirte
2010-06-15 0:08 ` Chris Mason
2010-06-16 18:04 ` Johannes Hirte
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).