On 12/14/2018 01:06 PM, Dmitry Safonov wrote: > On 12/13/18 10:10 PM, Waiman Long wrote: >> The bucket lock is for protecting the insertion and deletion of >> debug_obj to/from the bucket list as well as searching within the bucket >> list. It has nothing to do with the life time of the debug_obj itself. > The bucket lock also protects lookups on a bucket.
Yes, that is what I meant by "searching". > > 1. Imagine, you have object in ODEBUG_STATE_DESTROYED. > If you will try debug_object_activate() with this patch, it will debug > print object outside of bucket lock, which means that > debug_check_no_obj_freed() may concurrently fixup/free object and > meta-data on another CPU. > > I don't see this state is being used in many places around the kernel, > except selftest and i915 driver. > > 2. If you try to deactivate already non-active object - you will have > debug print outside of bucket lock. The other CPU can fixup/free page > with this object concurrently. > > (before your patch the lookup would fail and no printing) > > So, I might be mistaken, I'm mostly worried about dereferencing the > descriptor inside of the object during printing. > And in my opinion, i.e., there was a reason to save `descr` before > releasing the bucket lock here: > https://elixir.bootlin.com/linux/latest/source/lib/debugobjects.c#L789 > > Anyway, I see that your patch is already in -mm tree and Thomas seems to > be fine with the change so whatever %) I also thought about whether we should save descr before unlock. Anyway, we can always send a follow-up patch to make further change if it is the right thing to do. The debugobjects code is enabled in a debug kernel and never in a kernel for production use. The printk should only be triggered if there is a bug in the code which the debug object is tracking. Thanks, Longman
