netdev - Re: [RFC net-next 3/6] net: xdp: make the stack take care of the tear down

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]

Date:   Sat, 25 Nov 2017 00:24:50 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev@...r.kernel.org
Cc:     oss-drivers@...ronome.com, alexei.starovoitov@...il.com,
        jiri@...nulli.us, Saeed Mahameed <saeedm@...lanox.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [RFC net-next 3/6] net: xdp: make the stack take care of the tear
 down

On 11/24/2017 03:36 AM, Jakub Kicinski wrote:
> Since day one of XDP drivers had to remember to free the program
> on the remove path.  This leads to code duplication and is error
> prone.  Make the stack query the installed programs on unregister
> and if something is installed, remove the program.
> 
> Because the remove will now be called before notifiers are
> invoked, BPF offload state of the program will not get destroyed
> before uninstall.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Simon Horman <simon.horman@...ronome.com>
[...]

Nice work, series looks good to me! One really just minor
comment below:

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3f271c9cb5e0..a3e932f98419 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7110,6 +7110,23 @@ static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
>  	return bpf_op(dev, &xdp);
>  }
>  
> +static void dev_xdp_uninstall(struct net_device *dev)
> +{
> +	struct netdev_bpf xdp;
> +	bpf_op_t ndo_bpf;

Can you add a comment here stating that generic XDP does not
need to be handled since we drop the prog from free_netdev()?
Potentially we could also drop the generic one from here, that
way we'd make no difference and have a dev_xdp_install() and
one dev_xdp_uninstall() for all kind of attach types. Given
generic XDP should simulate native XDP anyway, probably better
to just do that.

> +	ndo_bpf = dev->netdev_ops->ndo_bpf;
> +	if (!ndo_bpf)
> +		return;
> +
> +	__dev_xdp_query(dev, ndo_bpf, &xdp);
> +	if (xdp.prog_attached == XDP_ATTACHED_NONE)
> +		return;
> +
> +	/* Program removal should always succeed */
> +	WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags, NULL));
> +}
> +
>  /**
>   *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
>   *	@dev: device
> @@ -7240,6 +7257,7 @@ static void rollback_registered_many(struct list_head *head)
>  		/* Shutdown queueing discipline. */
>  		dev_shutdown(dev);
>  
> +		dev_xdp_uninstall(dev);
>  
>  		/* Notify protocols, that we are about to destroy
>  		 * this device. They should clean all the things.
> 

Thanks,
Daniel