Re: [PATCH libnetfilter_queue] src: make nfq_open_nfnl thread-safe
On Mon, Jun 20, 2016 at 02:52:27PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso wrote: > > On Mon, Jun 20, 2016 at 02:42:59PM +0200, Pablo Neira Ayuso wrote: > > > On Mon, Jun 20, 2016 at 01:19:52PM +0200, Florian Westphal wrote: > > > > nfq_open_nfnl uses an intermediate static object, so when > > > > it is invoked by distinct threads at the same time there is a small > > > > chance that some threads end up with another threads nfq_handle pointer > > > > stored in ->data. > > > > > > > > Tested-by: Michal Tesar > > > > Signed-off-by: Florian Westphal > > > > > > Acked-by: Pablo Neira Ayuso > > > > Wait wait... > > Sorry, already pushed it after seeing the Ack... > > > This is allocated in this stack? > > > > struct nfq_handle *nfq_open_nfnl(struct nfnl_handle *nfnlh) > > { > > + struct nfnl_callback pkt_cb = { > > + .call = __nfq_rcv_pkt, > > + .attr_count = NFQA_MAX, > > + }; > > > > Then accessed out of it? > > > > I mean, this is passed by reference to the callback registration. > > AFAICS nfnl_callback_register() memcpy's the thing into nfnl_handle, > which is malloc'd in nfq_open_nfnl. Right, sorry for the noise. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libnetfilter_queue] src: make nfq_open_nfnl thread-safe
Pablo Neira Ayuso wrote: > On Mon, Jun 20, 2016 at 02:42:59PM +0200, Pablo Neira Ayuso wrote: > > On Mon, Jun 20, 2016 at 01:19:52PM +0200, Florian Westphal wrote: > > > nfq_open_nfnl uses an intermediate static object, so when > > > it is invoked by distinct threads at the same time there is a small > > > chance that some threads end up with another threads nfq_handle pointer > > > stored in ->data. > > > > > > Tested-by: Michal Tesar > > > Signed-off-by: Florian Westphal > > > > Acked-by: Pablo Neira Ayuso > > Wait wait... Sorry, already pushed it after seeing the Ack... > This is allocated in this stack? > > struct nfq_handle *nfq_open_nfnl(struct nfnl_handle *nfnlh) > { > + struct nfnl_callback pkt_cb = { > + .call = __nfq_rcv_pkt, > + .attr_count = NFQA_MAX, > + }; > > Then accessed out of it? > > I mean, this is passed by reference to the callback registration. AFAICS nfnl_callback_register() memcpy's the thing into nfnl_handle, which is malloc'd in nfq_open_nfnl. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libnetfilter_queue] src: make nfq_open_nfnl thread-safe
On Mon, Jun 20, 2016 at 02:42:59PM +0200, Pablo Neira Ayuso wrote: > On Mon, Jun 20, 2016 at 01:19:52PM +0200, Florian Westphal wrote: > > nfq_open_nfnl uses an intermediate static object, so when > > it is invoked by distinct threads at the same time there is a small > > chance that some threads end up with another threads nfq_handle pointer > > stored in ->data. > > > > Tested-by: Michal Tesar > > Signed-off-by: Florian Westphal > > Acked-by: Pablo Neira Ayuso Wait wait... This is allocated in this stack? struct nfq_handle *nfq_open_nfnl(struct nfnl_handle *nfnlh) { + struct nfnl_callback pkt_cb = { + .call = __nfq_rcv_pkt, + .attr_count = NFQA_MAX, + }; Then accessed out of it? I mean, this is passed by reference to the callback registration. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libnetfilter_queue] src: make nfq_open_nfnl thread-safe
On Mon, Jun 20, 2016 at 01:19:52PM +0200, Florian Westphal wrote: > nfq_open_nfnl uses an intermediate static object, so when > it is invoked by distinct threads at the same time there is a small > chance that some threads end up with another threads nfq_handle pointer > stored in ->data. > > Tested-by: Michal Tesar > Signed-off-by: Florian Westphal Acked-by: Pablo Neira Ayuso -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH libnetfilter_queue] src: make nfq_open_nfnl thread-safe
nfq_open_nfnl uses an intermediate static object, so when it is invoked by distinct threads at the same time there is a small chance that some threads end up with another threads nfq_handle pointer stored in ->data. Tested-by: Michal Tesar Signed-off-by: Florian Westphal --- src/libnetfilter_queue.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libnetfilter_queue.c b/src/libnetfilter_queue.c index 740b340..ce16f95 100644 --- a/src/libnetfilter_queue.c +++ b/src/libnetfilter_queue.c @@ -216,11 +216,6 @@ static int __nfq_rcv_pkt(struct nlmsghdr *nlh, struct nfattr *nfa[], return qh->cb(qh, nfmsg, &nfqa, qh->data); } -static struct nfnl_callback pkt_cb = { - .call = &__nfq_rcv_pkt, - .attr_count = NFQA_MAX, -}; - /* public interface */ struct nfnl_handle *nfq_nfnlh(struct nfq_handle *h) @@ -389,6 +384,10 @@ EXPORT_SYMBOL(nfq_open); */ struct nfq_handle *nfq_open_nfnl(struct nfnl_handle *nfnlh) { + struct nfnl_callback pkt_cb = { + .call = __nfq_rcv_pkt, + .attr_count = NFQA_MAX, + }; struct nfq_handle *h; int err; -- 2.7.3 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html