Re: oops in file_storage.c

[Alan Stern]

On Wed, 22 Oct 2008, Manish Lachwani wrote:

> Hi Alan,
> 
> I am using the 2.6.27 kernel on a ARM platform. In a test that
> transfers big files over the gadget interface, i occasionally run into
> an oops:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000003c
> Internal error: Oops: 17 [#1] PREEMPT
> Modules linked in: ppp_deflate ppp_async crc_ccitt ppp_generic slhc
> mxc_alsa_pmic snd_pcm snd_page_a
> lloc snd_timer snd fiveway mxc_keyb ip_queue iptable_filter ip_tables
> x_tables uinput fuse g_file_st
> orage mwan arcotg_udc charger eink_fb_pnlcd eink_fb_shim
> eink_fb_hal_broads eink_fb_hal
> CPU: 0 Not tainted (2.6.22.19-lab126 #4)
> PC is at fsync_sub+0x70/0xb4 [g_file_storage]
...

> So far, I am running into this issue only when I am trying to copy a
> large file when the device disk is almost full.

Odd.  Maybe this has to do with whether or not fsync_sub() gets called.

> The crash looks to occur at:
> 
>  e0c:       e593f03c        ldr     pc, [r3, #60]
> 
> this seems to be loading the filp->f_op->fsync into PC.
> 
> "Unable to handle kernel NULL pointer dereference at virtual address
> 0000003c" points to offset 60 as well.
> 
> However, there is a check for filp->f_op->fsync at the begining of the fn:
> 
>         if (!filp->f_op->fsync)
>                 return -EINVAL;
> 
> I noticed in fs/sync.c, similar check as part of sys_fdatasync:
> 
> if (!file->f_op || !file->f_op->fsync) {
> 
> I modified the code in fsync_sub() to do this and so far, no crash. Do
> you think this makes sense?
> 
> diff --git a/drivers/usb/gadget/file_storage.c
> b/drivers/usb/gadget/file_storage.c
> index c4e62a6..46db662 100644
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -1859,7 +1859,7 @@ static int do_write(struct fsg_dev *fsg)
>  /*-------------------------------------------------------------------------*/
> 
>  /* Sync the file data, don't bother with the metadata.
> - * This code was copied from fs/buffer.c:sys_fdatasync(). */
> + * This code was copied from fs/sync.c:sys_fdatasync(). */
>  static int fsync_sub(struct lun *curlun)
>  {
>         struct file     *filp = curlun->filp;
> @@ -1868,7 +1868,7 @@ static int fsync_sub(struct lun *curlun)
> 
>         if (curlun->ro || !filp)
>                 return 0;
> -       if (!filp->f_op->fsync)
> +       if (!filp->f_op || !filp->f_op->fsync)
>                 return -EINVAL;
> 
>         inode = filp->f_path.dentry->d_inode;

Hmm.  On my system (2.6.27) the code has moved to fs/sync.c/do_fsync(),
and it has changed a fair amount.  Maybe file_storage.c should be 
changed to match.

In fact, the easiest approach would be to EXPORT do_fsync() and then 
call it directly.  I don't know whether people would like this, though.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/