using sparse to catch refcount leaks

using sparse to catch refcount leaks

[Aurélien Aptel]

Hi,

I was thinking the same mechanism for lock checks (lock() has matching
unlock()) could be used for detecting refcount leaks (get() has matching
put()).

This could be used to catch bugs like that:

https://lore.kernel.org/linux-cifs/CAH2r5mtJg0OONLuAYmcggj=M3euDDxRa3Y5-_W1=qxwbeZypqA@mail.gmail.com/T/#mf0e0397aa0b63043d7b3bb0981f0b7323713bfdc

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: using sparse to catch refcount leaks

[Luc Van Oostenryck]

On Thu, Jul 09, 2020 at 05:50:11PM +0200, Aurélien Aptel wrote:
> Hi,
> 
> I was thinking the same mechanism for lock checks (lock() has matching
> unlock()) could be used for detecting refcount leaks (get() has matching
> put()).
> 
> This could be used to catch bugs like that:
> 
> https://lore.kernel.org/linux-cifs/CAH2r5mtJg0OONLuAYmcggj=M3euDDxRa3Y5-_W1=qxwbeZypqA@mail.gmail.com/T/#mf0e0397aa0b63043d7b3bb0981f0b7323713bfdc

Absolutely.
Sparse's 'context' can be reused for refcounting (conceptually,
it's quite similar to locking). You can even simply reuse the
existing macros __acquires(), ...

That's said, I suppose that in the present case the 'get' is done
by cifs_sb_tlink(). This corresponds to a 'conditional context'
(see __cond_lock()) which somehow annoying to use. I've some
patches improving things here but they need some more work.

Cheers,
-- Luc

Re: using sparse to catch refcount leaks

[Dan Carpenter]

On Thu, Jul 09, 2020 at 05:50:11PM +0200, Aurélien Aptel wrote:
> Hi,
> 
> I was thinking the same mechanism for lock checks (lock() has matching
> unlock()) could be used for detecting refcount leaks (get() has matching
> put()).
> 
> This could be used to catch bugs like that:
> 
> https://lore.kernel.org/linux-cifs/CAH2r5mtJg0OONLuAYmcggj=M3euDDxRa3Y5-_W1=qxwbeZypqA@mail.gmail.com/T/#mf0e0397aa0b63043d7b3bb0981f0b7323713bfdc

You might be better off copying the Smatch's locking test instead of
Sparse's.  Smatch does cross function flow analysis which can help.

In this CIFS case, the leak is on the success path (as well as the
failure path) so my theory would be that it would be caught in testing
and will only cause false positives for static analysis.  I can't see
any automated way to know which success paths should take a reference
and which should not.

regards,
dan carpenter

Re: using sparse to catch refcount leaks

[Aurélien Aptel]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> In this CIFS case, the leak is on the success path (as well as the
> failure path) so my theory would be that it would be caught in testing
> and will only cause false positives for static analysis.  I can't see
> any automated way to know which success paths should take a reference
> and which should not.

Ah, I see. I guess with extra annotations and more sophisticated
analysis we could have something but it seems out of scope.

Anyway, thanks for looking into it.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)