On Wed, Jun 12, 2024 at 12:21:41AM +0200, Pablo Neira Ayuso wrote: > On Tue, Jun 11, 2024 at 12:46:39PM +1000, Duncan Roe wrote: > > Hi Pablo, > > > > On Wed, Jun 05, 2024 at 05:19:23PM +0200, Pablo Neira Ayuso wrote: > > > Hi Duncan, > > > > > > On Tue, May 07, 2024 at 09:17:19AM +1000, Duncan Roe wrote: > > > > 0c5e5fb introduced struct nfqnl_q_handle *qh_list which can point to > > > > dynamically acquired memory. Without this patch, that memory is not freed. > > > > > > Indeed. > > > > > > Looking at the example available at utils, I can see this assumes > > > that: > > > > > > nfq_destroy_queue(qh); > > > > > > needs to be called. > > > > > > qh->data can be also set to heap structure, in that case this would leak too. > > > > > > It seems nfq_destroy_queue() needs to be called before nfq_close() by design. > > > > Oh sorry, I missed that. Anyone starting with the example available at utils as > > a template will be OK then. > > But someone carefully checking each line of code might do a > > `man nfq_destroy_queue` and see: > > Removes the binding for the specified queue handle. This call also > > unbind from the nfqueue handler, so you don't have to call > > nfq_unbind_pf. > > And on then doing `man nfq_unbind_pf` that person would see: > > Unbinds the given queue connection handle from processing packets > > belonging to the given protocol family. > > > > This call is obsolete, Linux kernels from 3.8 onwards ignore it. > > And might draw the conclusion that the call to nfq_destroy_queue is unnecessary, > > especially if planning to call exit after calling nfq_close. > > Then, update documentation. > > > > Probably add: > > > > > > assert(h->qh_list == NULL); > > > > I don't like that. It would be the first assert() in libnetfilter_queue. > > libnfnetlink is peppered with asserts: I removed them in the replacement > > libmnl-using code because libmnl doesn't have them. Have you looked at the v2 > > patches BTW? I'd really appreciate some feedback. > > > > > > > > at the top of nfq_close() instead to give a chance to users of this to > > > fix their code in case they are leaking qh? > > > > It's not as important to call nfq_destroy_queue as it used to be. Why not just > > free the memory? > > It is not possible to know if qh->data is stored in the bss, onstack > or the heap, it is up to the user to decide this. qh->data is a pointer which is assigned in nfq_create_queue() at libnetfilter_queue.c:584. I was never proposing to free what qh->data points to, only to free any left-over qh structs. The user cannot access qh->data directly because qh (struct nfq_q_handle) is opaque. nfq_destroy_queue(qh) will free qh at libnetfilter_queue.c:619. I'm just proposing to free qh's for which nfq_destroy_queue was not called. In nfq_close(h), h->qh_list can only have struct nfq_q_handles if it has anything. > > > I could send a v2 with the Fixes: tag removed and a commit > > message that mentions the change is a backstop in case nfq_destroy_queue was not > > called. I can still do that. It's an enhancement now. > > > > Either way, `man nfq_destroy_queue` could be improved e.g.: > > Removes the binding for the specified queue handle. This call also > > releases associated internal memory. > > While being about it, how about removing the obsolete code snippet at the > > head of Library initialisation (that details calls to nfq_[un]bind_pf)? > > Perhaps a separate doc: patch? > > I'd suggest to address this by updating documentation. Yes it could do with updating. My first instinct would be to remove the nfq_unbind_pf comments and code snippet as I mentioned originally. Kernel 3.8 is way out of LTS. But, there have been quite recent emails on the lists from folks who are stuck with these old kernels owing to having proprietary closed-source binary blobs. I could leave these old comments and doxygen lines in but with extra lines highlighting they are for pre-3.8 only. Do you have a preferance? Cheers ... Duncan. > > Thanks. >
