[lkml]   [2008]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] utimensat() non-conformances and fixes [v3]
    > >> Here's a further version of the patch, against 2.6.26rc2, with the
    > >> 2008-05-19 git changes you sent me applied. This patch is based on
    > >> the draft patch you sent me. I've tested this version of the patch,
    > >> and it works as for all cases except the one mentioned below. But
    > >> note the following points:
    > >>
    > >> 1) I didn't make use of the code in notify_change() that checks
    > >> IS_IMMUTABLE() and IS_APPEND() (i.e., I did not add
    > >> ATTR_OWNER_CHECK to the mask in the controlling if statement).
    > >> Doing this can't easily be made to work for the
    > >> do_futimes() case without reworking the arguments passed to
    > >> notify_change(). Actually, I'm inclined to doubt whether it
    > >> is a good idea to try to roll that check into notify_change() --
    > >> at least for utimensat() it seems simpler to not do so.
    > >
    > > Ugh... Could we just omit this part (the if !times and write error
    > > then check owner)? I know it was my idea, but
    > >
    > > a) my ideas are often stupid
    > > b) one patch should ideally do just one thing
    > >
    > > After we fixed the original issue, we can still think about this other
    > > thing :)
    > Okay, by now quite a bit of my time has been wasted, and my patience
    > is starting to get a little thin.

    I understand your frustration, but actually I did say this the last
    time as well:

    "Sorry, that was just an idea, but since it's not as simple as it
    should be, I guess we should leave that till later. My main
    objection was against introducing more is_owner_or_cap() checks.
    Just doing the times == NULL case with ATTR_OWNER_CHECK should be

    Yeah, I may have been more explicit...

    > The relevant interfaces here are:
    > utimensat()
    > futimesat()
    > utime()
    > utimes()
    > futimens() -- because implemented in glibc via utimensat() with path==NULL.
    > * utime() and utimes() can't be affected by this point: they don't use
    > file descriptors.

    OK, you're right. I've overlooked this point.

    So as long as we believe that nobody is using the futime*() interfaces
    in the way you described, which is highly probable, then we can fix
    that as well. Which is actually a nice thing, because it means the
    permission checking for the times == NULL case can move from both
    do_utimes_name() and do_futimes() into utimes_common().

    So let's make two patches, and let's forget about the write_error
    thing for now:

    - turn UTIME_NOW/UTIME_NOW into times = NULL
    - for times != NULL set ATTR_OWNER_CHECK

    - move times == NULL permission check into utimes_common.

    For 2) you can just use permission() instead of vfs_permission() which
    is exactly the same in this case (and consolidated into a common
    function later in the vfs-cleanups tree).



     \ /
      Last update: 2008-05-30 21:25    [W:0.021 / U:12.728 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site