From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753502AbdARSy2 (ORCPT ); Wed, 18 Jan 2017 13:54:28 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:37168 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752512AbdARSy0 (ORCPT ); Wed, 18 Jan 2017 13:54:26 -0500 MIME-Version: 1.0 In-Reply-To: <1484630048-25416-2-git-send-email-cernekee@chromium.org> References: <1484630048-25416-1-git-send-email-cernekee@chromium.org> <1484630048-25416-2-git-send-email-cernekee@chromium.org> From: Doug Anderson Date: Wed, 18 Jan 2017 10:54:24 -0800 X-Google-Sender-Auth: fiWJz_J1obKQh9gEbJUJhJFadCE Message-ID: Subject: Re: [RFC/PATCH 1/3] netfilter: ctnetlink: Fix regression in CTA_TIMEOUT processing To: Kevin Cernekee Cc: pablo@netfilter.org, netfilter-devel@vger.kernel.org, "linux-kernel@vger.kernel.org" , David Miller Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, Jan 16, 2017 at 9:14 PM, Kevin Cernekee wrote: > Commit b7bd1809e078 ("netfilter: nfnetlink_queue: get rid of > nfnetlink_queue_ct.c") introduced a new check on the return value > from the NFQA_CT parser (currently ctnetlink_glue_parse_ct()). > Prior to Linux 4.4, nfqnl_ct_parse() would process the NFQA_EXP > attribute even if there were errors in the NFQA_CT attribute. > After Linux 4.4, this is no longer true, so any error in the NFQA_CT > attribute will cause the kernel to silently fail to create an > expectation. > > The new check is causing user conntrack helpers to fail. If a user > program sends an NFQA_CT attribute containing a CTA_TIMEOUT attribute > before the connection is confirmed (i.e. before the initial > ACCEPT/DROP decision has been made), del_timer() in > ctnetlink_change_timeout() will fail, and all further processing will be > aborted. The (simplified) calling sequence looks like: > > nfnetlink_rcv_msg > nfqnl_recv_verdict > nfqnl_ct_parse > ctnetlink_glue_parse_ct > ctnetlink_change_timeout > del_timer [ERROR] > nf_reinject > __nf_conntrack_confirm > > Fix this by adding a case to ctnetlink_change_timeout() to handle > unconfirmed connections. > > Also, if a timeout of 0 is set for an unconfirmed connection, restore the > old behavior of ignoring it (rather than setting up a connection that > expires immediately). > > Signed-off-by: Kevin Cernekee > --- > net/netfilter/nf_conntrack_netlink.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) I'm not an expert (I'd never looked at netlink code before yesterday), but Kevin's explanation and proposal seem sane to me. In case it's helpful: Reviewed-by: Douglas Anderson