linux-kernel - Re: [PATCH 4/11] LTTng-core 0.5.108 : core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20060914140459.GA23823@sergelap.austin.ibm.com>
Date:	Thu, 14 Sep 2006 09:04:59 -0500
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc:	linux-kernel@...r.kernel.org,
	Christoph Hellwig <hch@...radead.org>,
	Andrew Morton <akpm@...l.org>, Ingo Molnar <mingo@...hat.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Tom Zanussi <zanussi@...ibm.com>, ltt-dev@...fik.org,
	Michel Dagenais <michel.dagenais@...ymtl.ca>
Subject: Re: [PATCH 4/11] LTTng-core 0.5.108 : core

Hi,

I'm wondering why this is safe:

Quoting Mathieu Desnoyers (mathieu.desnoyers@...ymtl.ca):
...
> +void ltt_transport_register(struct ltt_transport *transport)
> +{
> +	spin_lock(&transport_list_lock);
> +	list_add_tail(&transport->node, &ltt_transport_list);
> +	spin_unlock(&transport_list_lock);
> +}
> +
> +void ltt_transport_unregister(struct ltt_transport *transport)
> +{
> +	spin_lock(&transport_list_lock);
> +	list_del(&transport->node);

You don't have a refcount you check here, and

> +	spin_unlock(&transport_list_lock);
> +}
> +
> +EXPORT_SYMBOL_GPL(ltt_transport_register);
> +EXPORT_SYMBOL_GPL(ltt_transport_unregister);

...

> +static int ltt_trace_create(char *trace_name, char *trace_type,
> +		enum trace_mode mode,
> +		unsigned subbuf_size_low, unsigned n_subbufs_low,
> +		unsigned subbuf_size_med, unsigned n_subbufs_med,
> +		unsigned subbuf_size_high, unsigned n_subbufs_high)
> +{

here:

> +	spin_lock(&transport_list_lock);
> +	list_for_each_entry(tran, &ltt_transport_list, node) {
> +		if (!strcmp(tran->name, trace_type)) {
> +			transport = tran;
> +			break;
> +		}
> +	}
> +	spin_unlock(&transport_list_lock);
> +
> +	if (!transport) {
> +		err = EINVAL;
> +		printk(KERN_ERR	"LTT : Transport %s is not present.\n", trace_type);
> +		goto trace_error;
> +	}
> +
> +	if(!try_module_get(transport->owner)) {
> +		err = ENODEV;
> +		printk(KERN_ERR	"LTT : Can't lock transport module.\n");
> +		goto trace_error;
> +	}
> +
> +	new_trace->transport = transport;
> +	new_trace->ops = &transport->ops;

you grab references to the object which may be deleted after
you drop the transport_list_lock at the top of this block.  Since
a later patch shows the unregister being called right before the
owning module is unloaded, that seems awefuly dangerous.

Is there some other magic going on making this safe?

thanks,
-serge
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/