On Wed, 20 Apr 2011 12:27:31 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Wed, Apr 20, 2011 at 12:07 PM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
> > On Tue, 19 Apr 2011 08:21:09 -0500
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >>
> >> rb tree search and insertion routines.
> >>
> >> A SID which needs to be mapped, is looked up in one of the rb trees
> >> depending whether SID is either owner or group SID.
> >> If found in the tree, a (mapped) id from that node is assigned to
> >> uid or gid as appropriate. ÂIf unmapped, an upcall is attempted to
> >> map the SID to an id. ÂIf upcall is successful, node is marked as
> >> mapped. ÂIf upcall fails, node stays marked as unmapped and a mapping
> >> is attempted again only after an arbitrary time period has passed.
> >>
> >> To map a SID, which can be either a Owner SID or a Group SID, key
> >> description starts with the string "os" or "gs" followed by SID converted
> >> to a string. Without "os" or "gs", cifs.upcall does not know whether
> >> SID needs to be mapped to either an uid or a gid.
> >>
> >> Nodes in rb tree have fields to prevent multiple upcalls for
> >> a SID. ÂAdding and removing nodes is done within global locks.
> >> Shrinker routine prunes a node if it has expired but does not prne
> >> an expired node if its refcount is not zero (i.e. sid of that node
> >> is being mapped via an upcall).
> >> Every time an existing node is accessed, its timestamp is updated
> >> to prevent it from getting erased.
> >>
> >> For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
> >> it would be used to obtain an SID for an id.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> ---
> >> Âfs/cifs/cifsacl.c | Â332 +++++++++++++++++++++++++++++++++++++++++++++++++----
> >> Âfs/cifs/cifsacl.h | Â 25 ++++
> >> Â2 files changed, 333 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> >> index cad8d9b..bbc29b8 100644
> >> --- a/fs/cifs/cifsacl.c
> >> +++ b/fs/cifs/cifsacl.c
> >
> > [...]
> >
> >> + Â Â /*
> >> + Â Â Â* Shrinker could be accessing this entry but it will not
> >> + Â Â Â* get erased at this point in time and to make it stay further,
> >> + Â Â Â* pretty soon we will take a reference on it only to
> >> + Â Â Â* release the reference only after upcall returns (or we are
> >> + Â Â Â* woken up), however longer that upcall may take to return.
> >> + Â Â Â*/
> >> + Â Â if (!test_and_set_bit(SID_ID_PENDING, &psidid->state)) {
> >> + Â Â Â Â Â Â /* paidid and its fields are being accessed sans spinlock */
> >> + Â Â Â Â Â Â ++psidid->refcount;
> > Â Â Â Â Â Â Â Â^^^
> > Â Â Â Â Â Â Â ÂHuh? You're incrementing and decrementing the
> > Â Â Â Â Â Â Â Ârefcount without any locking at all? What prevents the
> > Â Â Â Â Â Â Â Âshrinker from finding the entry just before you bump
> > Â Â Â Â Â Â Â Âthe refcount and wiping it out anyway?
> >
>
> Becaue we just updated the timestamp so shrinker will not evict the
> entry at least for the duration of SID_MAP_EXPIRE.
> The refcount is to account for an lenghty upcall.
>
Making assumptions on timing like this is a recipe for difficult to
find bugs.
Why not increment the refcount as soon as you find the entry? Who's to
say that there won't be delays that prevent the increment from
happening before 1s is gone. That sort of thing can happen in a
preemptable kernel once you're outside of critical sections.
I think you need some clear locking rules around this refcount, and
ensure that there's never a window where the shrinker could delete the
object while it's still in use.
> >> +struct cifs_sid_id {
> >> + Â Â unsigned int refcount;
> >> + Â Â unsigned long id;
> >> + Â Â unsigned long time;
> >> + Â Â unsigned long retry;
> >
> > Why, exactly do we need "time" and "retry"? Wouldn't one timestamp
> > field do here?
> >
>
> One timestamp is to make an entry eligible for eviction and the other
> one is to attempt mapping again if previous attempt to map a sid had
> failed.
>
I think these can be condensed into one. The retry timestamp will be
unused once the upcall succeeds, and "time" doesn't really have any
meaning until the upcall succeeds.
> >> + Â Â unsigned long state;
> >> + Â Â char *sidstr;
> >> + Â Â struct rb_node rbnode;
> >> + Â Â struct cifs_sid sid;
> >> +};
> >> +
> >
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
