On Mon, Apr 27, 2015 at 02:35:02PM -0400, Oleg Drokin wrote:
>
> On Apr 27, 2015, at 1:38 PM, Dan Carpenter wrote:
>
> >> For example - the annoying false positives from functions that do:
> >> do_stuff(int lock)
> >> {
> >> if (lock)
> >> spin_lock(somelock);
> >>
> >> stuff;
> >>
> >> if (lock)
> >> spin_unlock(somelock);
> >> }
> >> I hope there's a way to get an idea of the preceding branch conditions and compare notes on lock/unlock
> >> for example?
> >
> > This example code shouldn't cause a false positive. smatch_implied.c
> > knows that the "if (lock)" condition implies that lock is set.
>
> I'd like to enter into evidence drivers/staging/lustre/lustre/lov/lov_io.c ;)
> that currently produces:
> drivers/staging/lustre/lustre/lov/lov_io.c:675 lov_io_submit() warn: 'mutex:&ld->ld_mutex' is sometimes locked here and sometimes unlocked.
>
> The code itself is:
> if (alloc) {
> …
> } else {
> ...
> mutex_lock(&ld->ld_mutex);
> lio->lis_mem_frozen = 1;
> }
> ….
> if (alloc) {
> OBD_FREE_LARGE(stripes_qin,
> sizeof(*stripes_qin) * lio->lis_nr_subios);
> } else {
>
> …
> lio->lis_mem_frozen = 0;
> mutex_unlock(&ld->ld_mutex);
>
> }
>
>
Yeah. It's sort of a known issue. Handling that particular code is
trickier than it looks.
> > I also recently made it so smatch_comparisons.c can set implications.
> > smatch_comparison.c is used for when we are comparing:
> >
> > if (unknown_variable <= other_unknown)
> > lock();
> >
> > What doesn't work is function calls:
> >
> > if (needs_lock(foo))
> >
> > That's tricky to add because of side effects. (Not impossibly tricky).
>
> I imagine we don't want to always ignore this anyway except for some well known
> functions.
>
> > Also "if (foo & NEEDS_LOCK)" doesn't work but it's similar to
> > smatch_comparison.c so I should be able to add that.
>
> More tricky is
> if (something->bitfield & mask)
> spin_lock();
>
> do_stuff(foo, something, somethingelse);
>
> if (something->bitfield & mask)
> spin_unlock();
>
> Here, we don't really know if the intermediate call happened to change the bitfield or not
> (unless we do know, due to a very simple call path or something).
The cross function analysis is actually pretty good here already. It
works.
>
> >> Any way to get the current function name to avoid false positives with aliased variable names?
> >
> > "cur_func" has the function name a string.
> > "cur_func_sym" has the function a symbol struct pointer.
> >
> > I'm not sure what "aliased variable names" means.
>
> it's stuff like this:
> /home/green/smt/git/lustre-release/lnet/klnds/o2iblnd/o2iblnd_cb.c:3448 kiblnd_scheduler() error: double lock 'irqsave:flags'
>
> spin_lock_irqsave(&sched->ibs_lock, flags);
> }
>
> kiblnd_conn_decref(conn); /* ...drop my ref from above */
>
> Where kiblnd_conn_decref is a macro that also does spin_lock_irqsave(…, flags), but despite the same name, it's a different one:
> #define kiblnd_conn_decref(conn) \
> do { \
> unsigned long flags; \
> ...
> spin_lock_irqsave(&kiblnd_data.kib_connd_lock, flags); \
>
> …
>
> or could be a similar problem with inlined functions too?
Ah. For most checks this is not a problem because we use the symbol
struct pointer and the variable name to identify the state. I have been
promising to re-write check_locking.c like that for a while. That would
let us use the cross function database as well.
>
> >> Additionally the current failure to print filename where the error is if we are dealing with
> >> a macro expansion from a header (it prints the correct line number, but wrong file).
> >
> > I'm not certain what you mean here. If it sees a macro() then it should
> > use the file and line of the .c file. I think you are talking about
> > macros which generate functions?
> >
> > Ah. I think you are still looking at --info output and that is not
> > meant for human consumption. The filename for static inline functions
> > needs to be the .c file for --info output but you shouldn't be looking
> > at that. For normal output it will use the .h filename as you would
> > expect.
>
> Hm, I ave not wrapped my head around this fully.
> Right now I runs this build_kernel_data.sh which builds the whole kernel
> (or external lustre tree) and gives me all the output I need.
> It did not occur to me that I need to then do make clean and rebuild
> once more.
>
> Hm… So I just did that and that completely removed all the "sometimes locked here and sometimes unlocked"
> warnings, certainly this should not be correct?
> Some other stuff disappeared too, like some of the "could be null",
> and all of the "potentially dereferencing uninitialized" messages - I know that at least one
> of them was valid:
> somefunc()
> {
> struct ptlrpc_request *req;
> …
> rc = ll_get_default_mdsize(sbi, &lmmsize);
> if (rc == 0)
> rc = md_getxattr(sbi->ll_md_exp, ll_inode2fid(inode), oc,
> OBD_MD_FLXATTR, XATTR_NAME_LOV, NULL, 0,
> lmmsize, 0, &req);
> capa_put(oc);
> if (rc < 0)
> RETURN(rc);
>
> body = req_capsule_server_get(&req->rq_pill, &RMF_MDT_BODY);
> …
>
> This one certainly shows in the build_kernel_data.sh build, but not in the test_kernel.sh output.
Those warnings are disabled by default because they have a high false
positive ratio. Use the --spammy option to turn them on.
>
> >> Oh, also the SQL statements - what are those for? just debug?
> >
> > If you cat all the SQL statements into a 17 GB warns.txt file and then
> > run `~/smatch/smatch_data/db/create_db.sh -p=kernel warns.txt` then it
> > creates a smatch_db.sqlite with call and return path information so you
> > can do cross function analysis.
>
> Is there an example of this cross function analysis, as in does it
> actually happen now anywhere? (I see the database is generated).
>
Yes. The second time you build the kernel then it uses the cross
function database. You can see what's in there with the smdb.py
script.
smatch/smatch_data/db/smdb.py lnet_build_msg_event
smatch/smatch_data/db/smdb.py return_states lnet_build_msg_event
If functions are very short then we do cross function analysis by
parsing them inline instead of using the db.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe smatch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
