> >> +{
> >> + struct klp_shadow *shadow;
> >> + unsigned long flags;
> >> +
> >> + shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags);
> >> + if (!shadow)
> >> + return NULL;
> >> +
> >> + shadow->obj = obj;
> >> + shadow->num = num;
> >> + if (new_data)
> >> + memcpy(shadow->new_data, new_data, new_size);
> >> +
> >> + if (lock)
> >> + spin_lock_irqsave(&klp_shadow_lock, flags);
> >> + hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj);
> >
> > We should check if the shadow variable already existed. Otherwise,
> > it would be possible to silently create many duplicates.
> >
> > It would make klp_shadow_attach() and klp_shadow_get_or_attach()
> > to behave the same.
>
> They would be almost exactly the same, except one version would bounce a
> redundant entry while the other would return the existing one. I could
> envision callers wanting any of the following behavior:
>
> If a shadow <obj, id> already exists:
> 0 - add a second shadow variable (??? why)
> 1 - return NULL, WARN
> 2 - return the existing one
> 3 - update the existing one with the new data and return it
>
> * v2 klp_shadow_attach() currently implements #0, can be made to do #1
> * v2 klp_shadow_get_or_attach() currently implements #2, but maybe #3
> makes more sense
I have a feeling that we're becoming overprotective here again. I think
that klp_shadow_attach() adding a new entry makes sense.
Although I can imagine #1. I think it is a responsibility of the user to
know what to call. And that is what klp_shadow_get_or_attach() is for.
klp_shadow_get() and klp_shadow_attach() are two main API functions.
klp_shadow_get_or_attach() is there to make things safe if needed
(concurrency).
> Going back to existing kpatch use-cases, since we paired shadow variable
> creation to their parent object creation, -EEXIST was never an issue. I
> think we concocted one proof-of-concept kpatch where we created shadow
> variables "in-flight", that is, we patched a routine that operated on
> the parent object and created a shadow variable if one did not already
> exist. The in-flight patch was for single function and we knew that it
> would never be called concurrently for the same parent object. tl;dr =
> kpatch never worried about existing shadow <obj, id>.
And this makes sense to me too.
> > I would do WARN() in klp_shadow_attach() when the variable
> > already existed are return NULL. Of course it might be inoncent
> > duplication. But it might mean that someone else is using another
> > variable of the same name but with different content. klp_shadow_get()
> > would then return the same variable for two different purposes.
> > Then the whole system might end like a glass on a stony floor.
>
> What do you think of expanding the API to include each the cases
> outlined above? Something like:
>
> 1 - klp_attach = allocate and add a unique <obj, id> to the hash,
> duplicates return NULL and a WARN
>
> 2 - klp_get_or_attach = return <obj, id> if it already exists,
> otherwise allocate a new one
>
> 3 - klp_get_or_update = update and return <obj, id> if it already
> exists, otherwise allocate a new one
>
> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should
> be dropped. Since you suggested adding klp_get_or_attach(), what do you
> think?
I don't know. I'd be prudent now. We can always add it later...
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
