Linux-Sparse Archive on lore.kernel.org
 help / color / Atom feed
* using sparse to catch refcount leaks
@ 2020-07-09 15:50 Aurélien Aptel
  2020-07-09 18:48 ` Luc Van Oostenryck
  2020-07-10  8:59 ` Dan Carpenter
  0 siblings, 2 replies; 4+ messages in thread
From: Aurélien Aptel @ 2020-07-09 15:50 UTC (permalink / raw)
  To: linux-sparse

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)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: using sparse to catch refcount leaks
  2020-07-09 15:50 using sparse to catch refcount leaks Aurélien Aptel
@ 2020-07-09 18:48 ` Luc Van Oostenryck
  2020-07-10  8:59 ` Dan Carpenter
  1 sibling, 0 replies; 4+ messages in thread
From: Luc Van Oostenryck @ 2020-07-09 18:48 UTC (permalink / raw)
  To: Aurélien Aptel; +Cc: linux-sparse

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: using sparse to catch refcount leaks
  2020-07-09 15:50 using sparse to catch refcount leaks Aurélien Aptel
  2020-07-09 18:48 ` Luc Van Oostenryck
@ 2020-07-10  8:59 ` Dan Carpenter
  2020-07-13 10:50   ` Aurélien Aptel
  1 sibling, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-07-10  8:59 UTC (permalink / raw)
  To: Aurélien Aptel; +Cc: linux-sparse

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: using sparse to catch refcount leaks
  2020-07-10  8:59 ` Dan Carpenter
@ 2020-07-13 10:50   ` Aurélien Aptel
  0 siblings, 0 replies; 4+ messages in thread
From: Aurélien Aptel @ 2020-07-13 10:50 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-sparse

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)

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 15:50 using sparse to catch refcount leaks Aurélien Aptel
2020-07-09 18:48 ` Luc Van Oostenryck
2020-07-10  8:59 ` Dan Carpenter
2020-07-13 10:50   ` Aurélien Aptel

Linux-Sparse Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sparse/0 linux-sparse/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sparse linux-sparse/ https://lore.kernel.org/linux-sparse \
		linux-sparse@vger.kernel.org
	public-inbox-index linux-sparse

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sparse


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git