lkml.org 
[lkml]   [2016]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] staging/lustre/llite: Use memdup_user() rather than duplicating its implementation
    From
    Date


    On Sunday 21 August 2016 04:31 PM, Julia Lawall wrote:
    >
    >
    > On Sun, 21 Aug 2016, Vaishali Thakkar wrote:
    >
    >>
    >>
    >> On Sunday 21 August 2016 04:01 PM, Julia Lawall wrote:
    >>>
    >>>
    >>> On Sun, 21 Aug 2016, Christophe JAILLET wrote:
    >>>
    >>>> Le 21/08/2016 à 11:45, SF Markus Elfring a écrit :
    >>>>> From: Markus Elfring <elfring@users.sourceforge.net>
    >>>>> Date: Sun, 21 Aug 2016 11:30:57 +0200
    >>>>>
    >>>>> Reuse existing functionality from memdup_user() instead of keeping
    >>>>> duplicate source code.
    >>>>>
    >>>>> This issue was detected by using the Coccinelle software.
    >>>>>
    >>>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
    >>>>> ---
    >>>>> drivers/staging/lustre/lustre/llite/dir.c | 12 +++---------
    >>>>> 1 file changed, 3 insertions(+), 9 deletions(-)
    >>>>>
    >>>>> diff --git a/drivers/staging/lustre/lustre/llite/dir.c
    >>>>> b/drivers/staging/lustre/lustre/llite/dir.c
    >>>>> index 031c9e4..8b70e42 100644
    >>>>> --- a/drivers/staging/lustre/lustre/llite/dir.c
    >>>>> +++ b/drivers/staging/lustre/lustre/llite/dir.c
    >>>>> @@ -1676,14 +1676,9 @@ out_poll:
    >>>>> case LL_IOC_QUOTACTL: {
    >>>>> struct if_quotactl *qctl;
    >>>>> - qctl = kzalloc(sizeof(*qctl), GFP_NOFS);
    >>>> Same as previously reported in another patch, GFP_NOFS has not the same
    >>>> meaning than GPF_KERNEL.
    >>>> So your proposed clean-up is not 100% equivalent.
    >>>>
    >>>> Are your sure that GPF_KERNEL instead of GFP_NOFS is right in this code?
    >>>>
    >>>> Maybe, the coccinelle check should be tweak to only spot "kzalloc(...,
    >>>> GFP_KERNEL)" allocation?
    >>>
    >>> To my dim recollection, GFP_NOFS is not actually allowed in a place where
    >>> copy_from_user is being used. copy_from_user can block due to page
    >>> faults, and GFP_NOFS is used when a certain kind of blocking is not
    >>> allowed. So if the code really needs GFP_NOFS, then something else is
    >>> wrong.
    >>>
    >>> The semantic patch intentionally does not specify GFP_KERNEL for this
    >>> reason, ie so that these issues will come up and be discussed. On the
    >>> ther hand I agree about the GFP_DMA case, since that doesn't relate to
    >>> blocking, as far as I know. The semantic patch should be updated to not
    >>> make/propose the change in that case.
    >>
    >> I think semantic patch should be updated for all possible flags except
    >> GFP_NOFS and GFP_ATOMIC. Because only using these 2 flags with
    >> copy_from_user can cause blocking.
    >
    > They don't cause blocking, but rather prevent it.

    Yes, sorry. I meant blocking functions which may sleep in between.

    > But people could use variables as well. Since other things are rare, it
    > seems like it could be better to only block reports on what is known to
    > be safe, and not to block reports on unknown things. A warning could be
    > given in some cases.

    Sounds reasonable. Warning will work I guess.

    > julia
    >
    >>
    >>> julia
    >>>
    >>>>
    >>>>> - if (!qctl)
    >>>>> - return -ENOMEM;
    >>>>> -
    >>>>> - if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
    >>>>> - rc = -EFAULT;
    >>>>> - goto out_quotactl;
    >>>>> - }
    >>>>> + qctl = memdup_user((void __user *)arg, sizeof(*qctl));
    >>>>> + if (IS_ERR(qctl))
    >>>>> + return PTR_ERR(qctl);
    >>>>> rc = quotactl_ioctl(sbi, qctl);
    >>>>> @@ -1691,7 +1686,6 @@ out_poll:
    >>>>> sizeof(*qctl)))
    >>>>> rc = -EFAULT;
    >>>>> -out_quotactl:
    >>>>> kfree(qctl);
    >>>>> return rc;
    >>>>> }
    >>>>
    >>>>
    >>>>
    >>>> ---
    >>>> L'absence de virus dans ce courrier électronique a été vérifiée par le
    >>>> logiciel antivirus Avast.
    >>>> https://www.avast.com/antivirus
    >>>>
    >>>>
    >>>> --
    >>>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
    >>>> the body of a message to majordomo@vger.kernel.org
    >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
    >>>>
    >>>
    >>
    >> --
    >> Vaishali
    >>
    >

    --
    Vaishali

    \
     
     \ /
      Last update: 2016-09-17 09:57    [W:4.146 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site