On Tue, Jul 15, 2014 at 05:47:18PM +0200, Pablo Neira Ayuso wrote: > This patch reworks the batching logic in several aspects: > > 1) New batch pages are now always added into the batch page list in > first place. Then, in the send path, if the last batch page is > empty, it is removed from the batch list. > > 2) nft_batch_page_add() is only called if the current batch page is > full. Therefore, it is guaranteed to find a valid netlink message > in the batch page when moving the tail that didn't fit into a new > batch page. > > 3) The batch paging is initialized and released from the nft_netlink() > path. > > 4) No more global struct mnl_nlmsg_batch *batch that points to the > current batch page. Instead, it is retrieved from the tail of the > batch list, which indicates the current batch page. > > This patch fixes a crash due to access of uninitialized memory area in > due to calling batch_page_add() with an empty batch in the send path, > and the memleak of the batch page contents. Reported in: > > http://patchwork.ozlabs.org/patch/367085/ > http://patchwork.ozlabs.org/patch/367774/ > > The patch is larger, but this save the zeroing of the batch page area. Tested, it works find. Just a little question: is it better to release all batches but the current after each message sending? > > Reported-by: Yanchuan Nian <ycnian@xxxxxxxxx> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> > --- > src/main.c | 14 +++++----- > src/mnl.c | 82 ++++++++++++++++++++++++++++++++------------------------- > src/netlink.c | 1 - > 3 files changed, 54 insertions(+), 43 deletions(-) > > diff --git a/src/main.c b/src/main.c > index 30ea2c6..552a35d 100644 > --- a/src/main.c > +++ b/src/main.c > @@ -173,6 +173,8 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs) > bool batch_supported = netlink_batch_supported(); > int ret = 0; > > + mnl_batch_init(); > + > batch_seqnum = mnl_batch_begin(); > list_for_each_entry(cmd, &state->cmds, list) { > memset(&ctx, 0, sizeof(ctx)); > @@ -182,16 +184,14 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs) > init_list_head(&ctx.list); > ret = do_command(&ctx, cmd); > if (ret < 0) > - return ret; > + goto out; > } > mnl_batch_end(); > > - if (mnl_batch_ready()) > - ret = netlink_batch_send(&err_list); > - else { > - mnl_batch_reset(); > + if (!mnl_batch_ready()) > goto out; > - } > + > + ret = netlink_batch_send(&err_list); > > list_for_each_entry_safe(err, tmp, &err_list, head) { > list_for_each_entry(cmd, &state->cmds, list) { > @@ -208,6 +208,8 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs) > } > } > out: > + mnl_batch_reset(); > + > list_for_each_entry_safe(cmd, next, &state->cmds, list) { > list_del(&cmd->list); > cmd_free(cmd); > diff --git a/src/mnl.c b/src/mnl.c > index e4f0339..42b11f5 100644 > --- a/src/mnl.c > +++ b/src/mnl.c > @@ -82,8 +82,6 @@ nft_mnl_talk(struct mnl_socket *nf_sock, const void *data, unsigned int len, > */ > #define BATCH_PAGE_SIZE getpagesize() * 32 > > -static struct mnl_nlmsg_batch *batch; > - > static struct mnl_nlmsg_batch *mnl_batch_alloc(void) > { > static char *buf; > @@ -93,11 +91,6 @@ static struct mnl_nlmsg_batch *mnl_batch_alloc(void) > return mnl_nlmsg_batch_start(buf, BATCH_PAGE_SIZE); > } > > -void mnl_batch_init(void) > -{ > - batch = mnl_batch_alloc(); > -} > - > static LIST_HEAD(batch_page_list); > static int batch_num_pages; > > @@ -106,35 +99,53 @@ struct batch_page { > struct mnl_nlmsg_batch *batch; > }; > > +void mnl_batch_init(void) > +{ > + struct batch_page *batch_page; > + > + batch_page = xmalloc(sizeof(struct batch_page)); > + batch_page->batch = mnl_batch_alloc(); > + batch_num_pages++; > + list_add_tail(&batch_page->head, &batch_page_list); > +} > + > +static struct batch_page *nft_batch_page_current(void) > +{ > + return list_entry(batch_page_list.prev, struct batch_page, head); > +} > + > static void *nft_nlmsg_batch_current(void) > { > - return mnl_nlmsg_batch_current(batch); > + return mnl_nlmsg_batch_current(nft_batch_page_current()->batch); > } > > -static void mnl_batch_page_add(void) > +static void nft_batch_page_add(void) > { > - struct batch_page *batch_page; > struct nlmsghdr *last_nlh; > > /* Get the last message not fitting in the batch */ > last_nlh = nft_nlmsg_batch_current(); > - > - batch_page = xmalloc(sizeof(struct batch_page)); > - batch_page->batch = batch; > - list_add_tail(&batch_page->head, &batch_page_list); > - batch_num_pages++; > - batch = mnl_batch_alloc(); > - > + /* Add new batch page */ > + mnl_batch_init(); > /* Copy the last message not fitting to the new batch page */ > memcpy(nft_nlmsg_batch_current(), last_nlh, last_nlh->nlmsg_len); > /* No overflow may happen as this is a new empty batch page */ > - mnl_nlmsg_batch_next(batch); > + mnl_nlmsg_batch_next(nft_batch_page_current()->batch); > +} > + > +static void nft_batch_page_release(struct batch_page *batch_page) > +{ > + list_del(&batch_page->head); > + xfree(mnl_nlmsg_batch_head(batch_page->batch)); > + mnl_nlmsg_batch_stop(batch_page->batch); > + xfree(batch_page); > + batch_num_pages--; > } > > static void nft_batch_continue(void) > { > - if (!mnl_nlmsg_batch_next(batch)) > - mnl_batch_page_add(); > + if (!mnl_nlmsg_batch_next(nft_batch_page_current()->batch)) > + nft_batch_page_add(); > } > > static uint32_t mnl_batch_put(int type) > @@ -171,12 +182,16 @@ bool mnl_batch_ready(void) > /* Check if the batch only contains the initial and trailing batch > * messages. In that case, the batch is empty. > */ > - return mnl_nlmsg_batch_size(batch) != (NLMSG_HDRLEN+sizeof(struct nfgenmsg)) * 2; > + return mnl_nlmsg_batch_size(nft_batch_page_current()->batch) != > + (NLMSG_HDRLEN+sizeof(struct nfgenmsg)) * 2; > } > > void mnl_batch_reset(void) > { > - mnl_nlmsg_batch_reset(batch); > + struct batch_page *batch_page, *next; > + > + list_for_each_entry_safe(batch_page, next, &batch_page_list, head) > + nft_batch_page_release(batch_page); > } > > static void mnl_err_list_node_add(struct list_head *err_list, int error, > @@ -226,12 +241,12 @@ static ssize_t mnl_nft_socket_sendmsg(const struct mnl_socket *nl) > .msg_iov = iov, > .msg_iovlen = batch_num_pages, > }; > - struct batch_page *batch_page, *next; > + struct batch_page *batch_page; > int i = 0; > > mnl_set_sndbuffer(nl); > > - list_for_each_entry_safe(batch_page, next, &batch_page_list, head) { > + list_for_each_entry(batch_page, &batch_page_list, head) { > iov[i].iov_base = mnl_nlmsg_batch_head(batch_page->batch); > iov[i].iov_len = mnl_nlmsg_batch_size(batch_page->batch); > i++; > @@ -243,10 +258,6 @@ static ssize_t mnl_nft_socket_sendmsg(const struct mnl_socket *nl) > sizeof(struct nfgenmsg)); > } > #endif > - list_del(&batch_page->head); > - xfree(batch_page->batch); > - xfree(batch_page); > - batch_num_pages--; > } > > return sendmsg(mnl_socket_get_fd(nl), &msg, 0); > @@ -262,12 +273,13 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list) > .tv_usec = 0 > }; > > - if (!mnl_nlmsg_batch_is_empty(batch)) > - mnl_batch_page_add(); > + /* Remove last page from the batch if it's empty */ > + if (mnl_nlmsg_batch_is_empty(nft_batch_page_current()->batch)) > + nft_batch_page_release(nft_batch_page_current()); > > ret = mnl_nft_socket_sendmsg(nl); > if (ret == -1) > - goto err; > + return -1; > > FD_ZERO(&readfds); > FD_SET(fd, &readfds); > @@ -275,14 +287,14 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list) > /* receive and digest all the acknowledgments from the kernel. */ > ret = select(fd+1, &readfds, NULL, NULL, &tv); > if (ret == -1) > - goto err; > + return -1; > > while (ret > 0 && FD_ISSET(fd, &readfds)) { > struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf; > > ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf)); > if (ret == -1) > - goto err; > + return -1; > > ret = mnl_cb_run(rcv_buf, ret, 0, portid, NULL, NULL); > /* Continue on error, make sure we get all acknowledgments */ > @@ -291,13 +303,11 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list) > > ret = select(fd+1, &readfds, NULL, NULL, &tv); > if (ret == -1) > - goto err; > + return -1; > > FD_ZERO(&readfds); > FD_SET(fd, &readfds); > } > -err: > - mnl_nlmsg_batch_reset(batch); > return ret; > } > > diff --git a/src/netlink.c b/src/netlink.c > index afad5a4..0176cb4 100644 > --- a/src/netlink.c > +++ b/src/netlink.c > @@ -59,7 +59,6 @@ static void __init netlink_open_sock(void) > { > nf_sock = nfsock_open(); > fcntl(mnl_socket_get_fd(nf_sock), F_SETFL, O_NONBLOCK); > - mnl_batch_init(); > } > > static void __exit netlink_close_sock(void) > -- > 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
