netdev - Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

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: <8661f50b-876f-50d8-f1a5-61b1dabc5398@gmail.com>
Date:   Fri, 1 Jun 2018 07:45:16 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tejaswi Tanikella <tejaswit@...eaurora.org>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports



On 06/01/2018 07:05 AM, Tejaswi Tanikella wrote:
> On receiving a IGMPv2/v3 query, based on max_delay set in the header a
> timer is started to send out a response after a random time within
> max_delay. If the system then moves into suspend state, Report is
> delayed until system wakes up.
> 
> In one reported scenario, on arm64 devices, max_delay was set to 10s,
> Reports were consistantly delayed if the timer is scheduled after 5 plus
> seconds.
> 
> Hold wakelock while starting the timer to prevent moving into suspend
> state.

I suppose this looks fine, but are not you going to be playing
whack-a-mole through the network stack wherever there are similar
patterns? Is not a better solution to move this to
in_dev_get()/in_dev_put() where you could create a proper wakelock per
network device? For instance, I could imagine ARP suffering from the
same short comings...

There is one thing that needs fixing though, see below.

> 
> Signed-off-by: Tejaswi Tanikella <tejaswit@...eaurora.org>
> ---
>  include/linux/igmp.h |  1 +b
>  net/ipv4/igmp.c      | 20 ++++++++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/igmp.h b/include/linux/igmp.h
> index f823185..9be1c58 100644
> --- a/include/linux/igmp.h
> +++ b/include/linux/igmp.h
> @@ -84,6 +84,7 @@ struct ip_mc_list {
>  	};
>  	struct ip_mc_list __rcu *next_hash;
>  	struct timer_list	timer;
> +	struct wakeup_source	*wakeup_src;

Since you are using this only when CONFIG_IP_MULTICAST is defined, you
might was well save a few bytes by making this enclosed within an ifdef
CONFIG_IP_MULTICAST here as well?

[snip]

> @@ -1415,6 +1429,8 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
>  #ifdef CONFIG_IP_MULTICAST
>  	timer_setup(&im->timer, igmp_timer_expire, 0);
>  	im->unsolicit_count = net->ipv4.sysctl_igmp_qrv;
> +	im->wakeup_src = wakeup_source_create("igmp_wakeup_source");

Missing error checking, wakeup_source_create() can return NULL here.
-- 
Florian