lockdep splat from fbmem - Kernel

This is a discussion on lockdep splat from fbmem - Kernel ; Hi, fb_mmap() is called under mmap_sem and acquires fb_info->lock. Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user() under it, which in turn might acquire mmap_sem. Just noticed the trace and wanted to let ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: lockdep splat from fbmem

  1. lockdep splat from fbmem

    Hi,

    fb_mmap() is called under mmap_sem and acquires fb_info->lock.

    Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
    mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
    under it, which in turn might acquire mmap_sem.

    Just noticed the trace and wanted to let you know.

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

  2. Re: lockdep splat from fbmem

    Johannes Weiner wrote:
    > Hi,
    >
    > fb_mmap() is called under mmap_sem and acquires fb_info->lock.
    >
    > Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
    > mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
    > under it, which in turn might acquire mmap_sem.
    >
    > Just noticed the trace and wanted to let you know.
    >
    > Hannes


    Good catch. We could try to push down the mutex and call all the
    copy_to/from_user() outside the lock. I can try look at it tonight,
    if someone doesn't fix it before.

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

  3. lockdep splat from ioctl and mmap fops sharing lock

    Johannes Weiner writes:

    > Hi,
    >
    > fb_mmap() is called under mmap_sem and acquires fb_info->lock.
    >
    > Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
    > mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
    > under it, which in turn might acquire mmap_sem.


    More affected drivers:

    agp_ioctl() doing usercopy under agp_fe.agp_mutex
    agp_mmap() taking agp_fe.agp_mutex under mmap_sem

    dv1394_write() doing usercopy under video->mtx
    dv1394_mmap() taking video->mtx under mmap_sem

    raw1394_ioctl() doing usercopy under fi->state_mutex
    raw1394_mmap() taking fi->state_mutex under mmap_sem

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

  4. Re: lockdep splat from ioctl and mmap fops sharing lock

    Johannes Weiner wrote:
    > Johannes Weiner writes:
    >
    >> Hi,
    >>
    >> fb_mmap() is called under mmap_sem and acquires fb_info->lock.
    >>
    >> Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
    >> mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
    >> under it, which in turn might acquire mmap_sem.

    >
    > More affected drivers:
    >
    > agp_ioctl() doing usercopy under agp_fe.agp_mutex
    > agp_mmap() taking agp_fe.agp_mutex under mmap_sem
    >
    > dv1394_write() doing usercopy under video->mtx
    > dv1394_mmap() taking video->mtx under mmap_sem


    OK, it doesn't surprise me that I didn't notice this one myself: The
    video->mtx usage is old, but dv1394 is not frequently used anymore; I
    for one don't test anything of it beyond loading and unloading.

    > raw1394_ioctl() doing usercopy under fi->state_mutex
    > raw1394_mmap() taking fi->state_mutex under mmap_sem


    The state_mutex in raw1394 however was introduced by me in patches
    written against 2.6.26 and 2.6.27-rcs. And I tested the ioctls and I'd
    like to think that I also tested mmaps. But maybe I didn't. Of course
    I ahve all sorts of lockdep options enabled.

    So, was the usage of mmap_sem changed after 2.6.27 or were my tests
    insufficient?
    --
    Stefan Richter
    -=====-==--- =-=- =-===
    http://arcgraph.de/sr/
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: lockdep splat from ioctl and mmap fops sharing lock

    Stefan Richter writes:

    > Johannes Weiner wrote:
    >> raw1394_ioctl() doing usercopy under fi->state_mutex
    >> raw1394_mmap() taking fi->state_mutex under mmap_sem

    >
    > The state_mutex in raw1394 however was introduced by me in patches
    > written against 2.6.26 and 2.6.27-rcs. And I tested the ioctls and I'd
    > like to think that I also tested mmaps. But maybe I didn't. Of course
    > I ahve all sorts of lockdep options enabled.
    >
    > So, was the usage of mmap_sem changed after 2.6.27 or were my tests
    > insufficient?


    In linux-next/-mm, copy_to/from_user have lockdep annotations telling
    that they might fault and therefor acquire the mmap_sem in #PF.

    But since faults on ioctl parameters are so rare, without these
    annotations you would probably never see a warning.

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

  6. Re: lockdep splat from ioctl and mmap fops sharing lock

    Johannes Weiner wrote:
    > Stefan Richter writes:
    >
    >> Johannes Weiner wrote:
    >>> raw1394_ioctl() doing usercopy under fi->state_mutex
    >>> raw1394_mmap() taking fi->state_mutex under mmap_sem

    ....
    >> was the usage of mmap_sem changed after 2.6.27 or were my tests
    >> insufficient?

    >
    > In linux-next/-mm, copy_to/from_user have lockdep annotations telling
    > that they might fault and therefor acquire the mmap_sem in #PF.
    >
    > But since faults on ioctl parameters are so rare, without these
    > annotations you would probably never see a warning.


    Thanks for the report and explanation. I logged this as
    http://bugzilla.kernel.org/show_bug.cgi?id=11823 (dv1394, won't fix) and
    http://bugzilla.kernel.org/show_bug.cgi?id=11824 (raw1394, to be fixed
    in the 2.6.28-rc timeframe since it is a post 2.6.27 regression).
    --
    Stefan Richter
    -=====-==--- =-=- ==--=
    http://arcgraph.de/sr/
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: lockdep splat from fbmem

    Johannes Weiner wrote:
    > Hi,
    >
    > fb_mmap() is called under mmap_sem and acquires fb_info->lock.
    >
    > Since 3e680aae4e53ab "fb: convert lock/unlock_kernel() into local fb
    > mutex", fb_ioctl() takes fb_info->lock and does copy_to/from_user()
    > under it, which in turn might acquire mmap_sem.
    >
    > Just noticed the trace and wanted to let you know.
    >
    > Hannes


    Johannes,

    could you check the following fix for fbmem? maybe some parts are still
    deadlockable, but I'm not able to raise any lockdep trace with it using
    the latest Linus' git.

    Thanks,
    -Andrea

    ---
    fb: avoid to call copy_from/to_user() with fb_info mutex held in fbmem ioctl()

    Signed-off-by: Andrea Righi
    ---
    drivers/video/fbcmap.c | 24 ++++++--
    drivers/video/fbmem.c | 150 ++++++++++++++++++++++++++++++------------------
    include/linux/fb.h | 5 +-
    3 files changed, 114 insertions(+), 65 deletions(-)

    diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
    index 91b78e6..8f46589 100644
    --- a/drivers/video/fbcmap.c
    +++ b/drivers/video/fbcmap.c
    @@ -245,14 +245,11 @@ int fb_set_cmap(struct fb_cmap *cmap, struct fb_info *info)
    return rc;
    }

    -int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
    +int fb_set_user_cmap(struct fb_cmap_user *cmap, int fbidx)
    {
    int rc, size = cmap->len * sizeof(u16);
    struct fb_cmap umap;
    -
    - if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
    - !info->fbops->fb_setcmap))
    - return -EINVAL;
    + struct fb_info *info;

    memset(&umap, 0, sizeof(struct fb_cmap));
    rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
    @@ -262,11 +259,24 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
    copy_from_user(umap.green, cmap->green, size) ||
    copy_from_user(umap.blue, cmap->blue, size) ||
    (cmap->transp && copy_from_user(umap.transp, cmap->transp, size))) {
    - fb_dealloc_cmap(&umap);
    - return -EFAULT;
    + rc = -EFAULT;
    + goto out;
    }
    umap.start = cmap->start;
    + info = get_fb_info(fbidx);
    + if (!info) {
    + rc = -ENODEV;
    + goto out;
    + }
    + if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
    + !info->fbops->fb_setcmap)) {
    + rc = -EINVAL;
    + goto out1;
    + }
    rc = fb_set_cmap(&umap, info);
    +out1:
    + put_fb_info(info);
    +out:
    fb_dealloc_cmap(&umap);
    return rc;
    }
    diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
    index cd5f20d..b4de3f3 100644
    --- a/drivers/video/fbmem.c
    +++ b/drivers/video/fbmem.c
    @@ -1002,6 +1002,24 @@ fb_blank(struct fb_info *info, int blank)
    return ret;
    }

    +struct fb_info *get_fb_info(int fbidx)
    +{
    + struct fb_info *info;
    +
    + info = registered_fb[fbidx];
    + mutex_lock(&info->lock);
    + if (!info->fbops) {
    + mutex_unlock(&info->lock);
    + return NULL;
    + }
    + return info;
    +}
    +
    +void put_fb_info(struct fb_info *info)
    +{
    + mutex_unlock(&info->lock);
    +}
    +
    static long
    fb_ioctl(struct file *file, unsigned int cmd,
    unsigned long arg)
    @@ -1013,120 +1031,138 @@ fb_ioctl(struct file *file, unsigned int cmd,
    struct fb_var_screeninfo var;
    struct fb_fix_screeninfo fix;
    struct fb_con2fbmap con2fb;
    + struct fb_cmap cmap_from;
    struct fb_cmap_user cmap;
    struct fb_event event;
    void __user *argp = (void __user *)arg;
    long ret = 0;

    - info = registered_fb[fbidx];
    - mutex_lock(&info->lock);
    - fb = info->fbops;
    -
    - if (!fb) {
    - mutex_unlock(&info->lock);
    - return -ENODEV;
    - }
    switch (cmd) {
    case FBIOGET_VSCREENINFO:
    - ret = copy_to_user(argp, &info->var,
    - sizeof(var)) ? -EFAULT : 0;
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    + memcpy(&var, &info->var, sizeof(var));
    + put_fb_info(info);
    +
    + ret = copy_to_user(argp, &var, sizeof(var)) ? -EFAULT : 0;
    break;
    case FBIOPUT_VSCREENINFO:
    - if (copy_from_user(&var, argp, sizeof(var))) {
    - ret = -EFAULT;
    - break;
    - }
    + if (copy_from_user(&var, argp, sizeof(var)))
    + return -EFAULT;
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    acquire_console_sem();
    info->flags |= FBINFO_MISC_USEREVENT;
    ret = fb_set_var(info, &var);
    info->flags &= ~FBINFO_MISC_USEREVENT;
    release_console_sem();
    + put_fb_info(info);
    if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
    - ret = -EFAULT;
    + return -EFAULT;
    break;
    case FBIOGET_FSCREENINFO:
    - ret = copy_to_user(argp, &info->fix,
    - sizeof(fix)) ? -EFAULT : 0;
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    + memcpy(&fix, &info->fix, sizeof(fix));
    + put_fb_info(info);
    +
    + ret = copy_to_user(argp, &fix, sizeof(fix)) ? -EFAULT : 0;
    break;
    case FBIOPUTCMAP:
    if (copy_from_user(&cmap, argp, sizeof(cmap)))
    - ret = -EFAULT;
    - else
    - ret = fb_set_user_cmap(&cmap, info);
    + return -EFAULT;
    + ret = fb_set_user_cmap(&cmap, fbidx);
    break;
    case FBIOGETCMAP:
    if (copy_from_user(&cmap, argp, sizeof(cmap)))
    - ret = -EFAULT;
    - else
    - ret = fb_cmap_to_user(&info->cmap, &cmap);
    + return -EFAULT;
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    + memcpy(&cmap_from, &info->cmap, sizeof(cmap_from));
    + put_fb_info(info);
    + ret = fb_cmap_to_user(&cmap_from, &cmap);
    break;
    case FBIOPAN_DISPLAY:
    - if (copy_from_user(&var, argp, sizeof(var))) {
    - ret = -EFAULT;
    - break;
    - }
    + if (copy_from_user(&var, argp, sizeof(var)))
    + return -EFAULT;
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    acquire_console_sem();
    ret = fb_pan_display(info, &var);
    release_console_sem();
    + put_fb_info(info);
    if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
    - ret = -EFAULT;
    + return -EFAULT;
    break;
    case FBIO_CURSOR:
    ret = -EINVAL;
    break;
    case FBIOGET_CON2FBMAP:
    if (copy_from_user(&con2fb, argp, sizeof(con2fb)))
    - ret = -EFAULT;
    - else if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
    - ret = -EINVAL;
    - else {
    - con2fb.framebuffer = -1;
    - event.info = info;
    - event.data = &con2fb;
    - fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP,
    - &event);
    - ret = copy_to_user(argp, &con2fb,
    - sizeof(con2fb)) ? -EFAULT : 0;
    - }
    + return -EFAULT;
    + if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
    + return -EINVAL;
    + con2fb.framebuffer = -1;
    + event.data = &con2fb;
    +
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    + event.info = info;
    + fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP, &event);
    + put_fb_info(info);
    +
    + ret = copy_to_user(argp, &con2fb, sizeof(con2fb)) ? -EFAULT : 0;
    break;
    case FBIOPUT_CON2FBMAP:
    - if (copy_from_user(&con2fb, argp, sizeof(con2fb))) {
    - ret = -EFAULT;
    - break;
    - }
    - if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES) {
    - ret = -EINVAL;
    - break;
    - }
    - if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX) {
    - ret = -EINVAL;
    - break;
    - }
    + if (copy_from_user(&con2fb, argp, sizeof(con2fb)))
    + return -EFAULT;
    + if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
    + return -EINVAL;
    + if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
    + return -EINVAL;
    if (!registered_fb[con2fb.framebuffer])
    request_module("fb%d", con2fb.framebuffer);
    if (!registered_fb[con2fb.framebuffer]) {
    ret = -EINVAL;
    break;
    }
    - event.info = info;
    event.data = &con2fb;
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    + event.info = info;
    ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP,
    &event);
    + put_fb_info(info);
    break;
    case FBIOBLANK:
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    acquire_console_sem();
    info->flags |= FBINFO_MISC_USEREVENT;
    ret = fb_blank(info, arg);
    info->flags &= ~FBINFO_MISC_USEREVENT;
    release_console_sem();
    + put_fb_info(info);
    break;;
    default:
    - if (fb->fb_ioctl == NULL)
    - ret = -ENOTTY;
    - else
    + info = get_fb_info(fbidx);
    + if (!info)
    + return -ENODEV;
    + fb = info->fbops;
    + if (fb->fb_ioctl)
    ret = fb->fb_ioctl(info, cmd, arg);
    + else
    + ret = -ENOTTY;
    + put_fb_info(info);
    }
    - mutex_unlock(&info->lock);
    return ret;
    }

    diff --git a/include/linux/fb.h b/include/linux/fb.h
    index 75a81ea..9f4e70f 100644
    --- a/include/linux/fb.h
    +++ b/include/linux/fb.h
    @@ -960,6 +960,9 @@ extern struct fb_info *registered_fb[FB_MAX];
    extern int num_registered_fb;
    extern struct class *fb_class;

    +struct fb_info *get_fb_info(int fbidx);
    +void put_fb_info(struct fb_info *info);
    +
    static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
    u8 *src, u32 s_pitch, u32 height)
    {
    @@ -1068,7 +1071,7 @@ extern void fb_dealloc_cmap(struct fb_cmap *cmap);
    extern int fb_copy_cmap(const struct fb_cmap *from, struct fb_cmap *to);
    extern int fb_cmap_to_user(const struct fb_cmap *from, struct fb_cmap_user *to);
    extern int fb_set_cmap(struct fb_cmap *cmap, struct fb_info *fb_info);
    -extern int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *fb_info);
    +extern int fb_set_user_cmap(struct fb_cmap_user *cmap, int fbidx);
    extern const struct fb_cmap *fb_default_cmap(int len);
    extern void fb_invert_cmaps(void);

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

+ Reply to Thread