* Re: [PATCH] dt-bindings: can: rcar_can: Fix R8A7796 SoC name
From: Marc Kleine-Budde @ 2018-05-05 13:56 UTC (permalink / raw)
To: Geert Uytterhoeven, Wolfgang Grandegger, Rob Herring,
Mark Rutland
Cc: Chris Paterson, linux-can, netdev, devicetree, linux-renesas-soc
In-Reply-To: <1525352553-17045-1-git-send-email-geert+renesas@glider.be>
[-- Attachment #1.1: Type: text/plain, Size: 461 bytes --]
On 05/03/2018 03:02 PM, Geert Uytterhoeven wrote:
> R8A7796 is R-Car M3-W.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Applied to linux-can.
thnx,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 38/40] ide: remove ide_driver_proc_write
From: Eric W. Biederman @ 2018-05-05 13:09 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, devel,
linux-kernel, linux-scsi, linux-ide, Greg Kroah-Hartman,
jfs-discussion, linux-afs, linux-acpi, netdev, netfilter-devel,
Alexander Viro, Jiri Slaby, Andrew Morton, linux-ext4,
Alexey Dobriyan, megaraidlinux.pdl, drbd-dev
In-Reply-To: <20180425154827.32251-39-hch@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> The driver proc file hasn't been writeable for a long time, so this is
> just dead code.
It is possible to chmod this file to get at the write method. Not that
I think anyone does.
It looks like this code was merged in 2.3.99-pre1 with permissions
S_IFREG|S_IRUGO so I don't think the write support was ever finished.
That cap_capable in the write method looks down right scary/buggy.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Eric
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/ide/ide-proc.c | 46 ------------------------------------------
> 1 file changed, 46 deletions(-)
>
> diff --git a/drivers/ide/ide-proc.c b/drivers/ide/ide-proc.c
> index 863db44c7916..b3b8b8822d6a 100644
> --- a/drivers/ide/ide-proc.c
> +++ b/drivers/ide/ide-proc.c
> @@ -528,58 +528,12 @@ static int ide_driver_proc_open(struct inode *inode, struct file *file)
> return single_open(file, ide_driver_proc_show, PDE_DATA(inode));
> }
>
> -static int ide_replace_subdriver(ide_drive_t *drive, const char *driver)
> -{
> - struct device *dev = &drive->gendev;
> - int ret = 1;
> - int err;
> -
> - device_release_driver(dev);
> - /* FIXME: device can still be in use by previous driver */
> - strlcpy(drive->driver_req, driver, sizeof(drive->driver_req));
> - err = device_attach(dev);
> - if (err < 0)
> - printk(KERN_WARNING "IDE: %s: device_attach error: %d\n",
> - __func__, err);
> - drive->driver_req[0] = 0;
> - if (dev->driver == NULL) {
> - err = device_attach(dev);
> - if (err < 0)
> - printk(KERN_WARNING
> - "IDE: %s: device_attach(2) error: %d\n",
> - __func__, err);
> - }
> - if (dev->driver && !strcmp(dev->driver->name, driver))
> - ret = 0;
> -
> - return ret;
> -}
> -
> -static ssize_t ide_driver_proc_write(struct file *file, const char __user *buffer,
> - size_t count, loff_t *pos)
> -{
> - ide_drive_t *drive = PDE_DATA(file_inode(file));
> - char name[32];
> -
> - if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> - if (count > 31)
> - count = 31;
> - if (copy_from_user(name, buffer, count))
> - return -EFAULT;
> - name[count] = '\0';
> - if (ide_replace_subdriver(drive, name))
> - return -EINVAL;
> - return count;
> -}
> -
> static const struct file_operations ide_driver_proc_fops = {
> .owner = THIS_MODULE,
> .open = ide_driver_proc_open,
> .read = seq_read,
> .llseek = seq_lseek,
> .release = single_release,
> - .write = ide_driver_proc_write,
> };
>
> static int ide_media_proc_show(struct seq_file *m, void *v)
^ permalink raw reply
* Re: [PATCH RFC net-next] net: ipvs: Adjust gso_size for IPPROTO_TCP
From: Julian Anastasov @ 2018-05-05 12:58 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: netdev, David Ahern, Tom Herbert, Eric Dumazet, Nikita Shirokov,
kernel-team, lvs-devel
In-Reply-To: <20180503070114.bcuusvzga45klccs@kafai-mbp>
Hello,
On Thu, 3 May 2018, Martin KaFai Lau wrote:
> > - when exactly we start to use the new PMTU, eg. what happens
> > in case socket caches the route, whether route is killed via
> > dst->obsolete. Or may be while the PMTU expiration is handled
> > per-packet, the PMTU change is noticed only on ICMP...
> Before sk can reuse its dst cache, the sk will notice
> its dst cache is no longer valid by calling dst_check().
> dst_check() should return NULL which is one of the side
> effect of the earlier update_pmtu(). This dst_check()
> is usually only called when the sk needs to do output,
> so the new PMTU route (i.e. the RTF_CACHE IPv6 route)
> only have effect to the later packets.
I checked again the code and it looks like sockets
are forced to use new exceptional route (RTF_CACHE/fnhe) via
dst_check only when the PMTU update should move them away
from old non-exceptional routes. Later, if PMTU is
reduced/updated this is noticed for every packet via dst_mtu,
as in the case with TCP.
So, except the RTF_LOCAL check in __ip6_rt_update_pmtu
we should have no other issues. Only one minor bit is strange to me,
why rt6_insert_exception warns for RTF_PCPU if rt6_cache_allowed_for_pmtu
allows it when returning true...
Also, commit 0d3f6d297bfb allows rt6_do_update_pmtu() for
routes without RTF_CACHE, RTF_PCPU and rt6i_node. Should we
restrict rt6_do_update_pmtu only to RTF_CACHE routes?
if (!rt6_cache_allowed_for_pmtu(rt6)) {
- rt6_do_update_pmtu(rt6, mtu);
- /* update rt6_ex->stamp for cache */
- if (rt6->rt6i_flags & RTF_CACHE)
+ if (rt6->rt6i_flags & RTF_CACHE) {
+ rt6_do_update_pmtu(rt6, mtu);
+ /* update rt6_ex->stamp for cache */
rt6_update_exception_stamp_rt(rt6);
+ }
} else if (daddr) {
Regards
^ permalink raw reply
* Re: [PATCH 34/40] atm: simplify procfs code
From: Eric W. Biederman @ 2018-05-05 12:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, devel,
linux-kernel, linux-scsi, linux-ide, Greg Kroah-Hartman,
jfs-discussion, linux-afs, linux-acpi, netdev, netfilter-devel,
Alexander Viro, Jiri Slaby, Andrew Morton, linux-ext4,
Alexey Dobriyan, megaraidlinux.pdl, drbd-dev
In-Reply-To: <20180425154827.32251-35-hch@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> Use remove_proc_subtree to remove the whole subtree on cleanup, and
> unwind the registration loop into individual calls. Switch to use
> proc_create_seq where applicable.
Can you please explain why you are removing the error handling when
you are unwinding the registration loop?
> int __init atm_proc_init(void)
> {
> - static struct atm_proc_entry *e;
> - int ret;
> -
> atm_proc_root = proc_net_mkdir(&init_net, "atm", init_net.proc_net);
> if (!atm_proc_root)
> - goto err_out;
> - for (e = atm_proc_ents; e->name; e++) {
> - struct proc_dir_entry *dirent;
> -
> - dirent = proc_create(e->name, 0444,
> - atm_proc_root, e->proc_fops);
> - if (!dirent)
> - goto err_out_remove;
> - e->dirent = dirent;
> - }
> - ret = 0;
> -out:
> - return ret;
> -
> -err_out_remove:
> - atm_proc_dirs_remove();
> -err_out:
> - ret = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> + proc_create_seq("devices", 0444, atm_proc_root, &atm_dev_seq_ops);
> + proc_create("pvc", 0444, atm_proc_root, &pvc_seq_fops);
> + proc_create("svc", 0444, atm_proc_root, &svc_seq_fops);
> + proc_create("vc", 0444, atm_proc_root, &vcc_seq_fops);
> + return 0;
> }
These proc entries could fail to register with -ENOMEM if for no other
reason. Can you justify the removal of the error handling?
Can you please at least mention that removal in your change description
and explain why it is reasonable.
As it sits this is not a semantics preserving transformation, and the
difference is not documented. Which raises red flags for me.
Eric
^ permalink raw reply
* Re: [PATCH v3 12/20] media: Remove depends on HAS_DMA in case of platform dependency
From: Mauro Carvalho Chehab @ 2018-05-05 12:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Ulf Hansson, Wolfram Sang, linux-iio, linux-fpga,
linux-remoteproc, alsa-devel, Bjorn Andersson, Eric Anholt,
netdev, linux-mtd, linux-i2c, linux1394-devel, Christoph Hellwig,
Marek Szyprowski, Stefan Wahren, Boris Brezillon, Herbert Xu,
Richard Weinberger, Joerg Roedel, Jassi Brar, Marek Vasut,
linux-serial, Matias Bjorling, David Woodhouse, linux-media, Ohad
In-Reply-To: <1523987360-18760-13-git-send-email-geert@linux-m68k.org>
Hi Geert,
Em Tue, 17 Apr 2018 19:49:12 +0200
Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> Remove dependencies on HAS_DMA where a Kconfig symbol depends on another
> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
Applying a patch like that is hard, as there are lots of churn at
the code. That's against latest media upstream:
checking file drivers/media/pci/intel/ipu3/Kconfig
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED
checking file drivers/media/pci/solo6x10/Kconfig
checking file drivers/media/pci/sta2x11/Kconfig
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED
checking file drivers/media/pci/tw5864/Kconfig
checking file drivers/media/pci/tw686x/Kconfig
checking file drivers/media/platform/Kconfig
Hunk #2 FAILED at 63.
Hunk #3 succeeded at 81 (offset 1 line).
Hunk #4 succeeded at 91 (offset 1 line).
Hunk #5 succeeded at 101 (offset 1 line).
Hunk #6 succeeded at 111 (offset 1 line).
Hunk #7 succeeded at 124 (offset 1 line).
Hunk #8 succeeded at 142 (offset 1 line).
Hunk #9 succeeded at 169 (offset 1 line).
Hunk #10 succeeded at 186 (offset 1 line).
Hunk #11 succeeded at 197 (offset 1 line).
Hunk #12 succeeded at 213 (offset 1 line).
Hunk #13 succeeded at 227 (offset 1 line).
Hunk #14 succeeded at 254 (offset 1 line).
Hunk #15 succeeded at 265 (offset 1 line).
Hunk #16 succeeded at 275 (offset 1 line).
Hunk #17 succeeded at 284 (offset 1 line).
Hunk #18 succeeded at 295 (offset 1 line).
Hunk #19 succeeded at 303 (offset 1 line).
Hunk #20 succeeded at 312 (offset 1 line).
Hunk #21 succeeded at 338 (offset 1 line).
Hunk #22 succeeded at 383 (offset 1 line).
Hunk #23 succeeded at 397 (offset 1 line).
Hunk #24 succeeded at 422 (offset 1 line).
Hunk #25 succeeded at 435 (offset 1 line).
Hunk #26 succeeded at 452 (offset 1 line).
Hunk #27 succeeded at 470 (offset 1 line).
Hunk #28 succeeded at 612 (offset 1 line).
1 out of 28 hunks FAILED
> In most cases this other symbol is an architecture or platform specific
> symbol, or PCI.
>
> Generic symbols and drivers without platform dependencies keep their
> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
> cannot work anyway.
Actually, depends on HAS_DMA was introduced on media because builds
were failing otherwise. We started adding it before the addition
of COMPILE_TEST.
Can we just remove all HAS_DMA Kconfig dependencies as a hole from the
entire media subsystem, with something like:
$ for i in $(find drivers/media -name Kconfig) $(find drivers/staging/media -name Kconfig); do sed '/depends on HAS_DMA/d;s/ && HAS_DMA//g' -i $i; done
Or would it cause build issues?
Regards,
Mauro
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH 11/40] ipv6/flowlabel: simplify pid namespace lookup
From: Eric W. Biederman @ 2018-05-05 12:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, Alexander Viro, Alexey Dobriyan,
Greg Kroah-Hartman, Jiri Slaby, Alessandro Zummo,
Alexandre Belloni, linux-acpi, drbd-dev, linux-ide, netdev,
linux-rtc, megaraidlinux.pdl, linux-scsi, devel, linux-afs,
linux-ext4, jfs-discussion, netfilter-devel, linux-kernel
In-Reply-To: <20180425154827.32251-12-hch@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> The shole seq_file sequence already operates under a single RCU lock pair,
> so move the pid namespace lookup into it, and stop grabbing a reference
> and remove all kinds of boilerplate code.
This is wrong.
Move task_active_pid_ns(current) from open to seq_start actually means
that the results if you pass this proc file between callers the results
will change. So this breaks file descriptor passing.
Open is a bad place to access current. In the middle of read/write is
broken.
In this particular instance looking up the pid namespace with
task_active_pid_ns was a personal brain fart. What the code should be
doing (with an appropriate helper) is:
struct pid_namespace *pid_ns = inode->i_sb->s_fs_info;
Because each mount of proc is bound to a pid namespace. Looking up the
pid namespace from the super_block is a much better way to go.
Eric
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> net/ipv6/ip6_flowlabel.c | 28 +++++-----------------------
> 1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> index c05c4e82a7ca..a9f221d45ef9 100644
> --- a/net/ipv6/ip6_flowlabel.c
> +++ b/net/ipv6/ip6_flowlabel.c
> @@ -754,7 +754,10 @@ static struct ip6_flowlabel *ip6fl_get_idx(struct seq_file *seq, loff_t pos)
> static void *ip6fl_seq_start(struct seq_file *seq, loff_t *pos)
> __acquires(RCU)
> {
> + struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
> +
> rcu_read_lock_bh();
> + state->pid_ns = task_active_pid_ns(current);
> return *pos ? ip6fl_get_idx(seq, *pos - 1) : SEQ_START_TOKEN;
> }
>
> @@ -810,36 +813,15 @@ static const struct seq_operations ip6fl_seq_ops = {
>
> static int ip6fl_seq_open(struct inode *inode, struct file *file)
> {
> - struct seq_file *seq;
> - struct ip6fl_iter_state *state;
> - int err;
> -
> - err = seq_open_net(inode, file, &ip6fl_seq_ops,
> + return seq_open_net(inode, file, &ip6fl_seq_ops,
> sizeof(struct ip6fl_iter_state));
> -
> - if (!err) {
> - seq = file->private_data;
> - state = ip6fl_seq_private(seq);
> - rcu_read_lock();
> - state->pid_ns = get_pid_ns(task_active_pid_ns(current));
> - rcu_read_unlock();
> - }
> - return err;
> -}
> -
> -static int ip6fl_seq_release(struct inode *inode, struct file *file)
> -{
> - struct seq_file *seq = file->private_data;
> - struct ip6fl_iter_state *state = ip6fl_seq_private(seq);
> - put_pid_ns(state->pid_ns);
> - return seq_release_net(inode, file);
> }
>
> static const struct file_operations ip6fl_seq_fops = {
> .open = ip6fl_seq_open,
> .read = seq_read,
> .llseek = seq_lseek,
> - .release = ip6fl_seq_release,
> + .release = seq_release_net,
> };
>
> static int __net_init ip6_flowlabel_proc_init(struct net *net)
^ permalink raw reply
* Re: [PATCH] net: dsa: drop some VLAs in switch.c
From: Salvatore Mesoraca @ 2018-05-05 10:36 UTC (permalink / raw)
To: Florian Fainelli
Cc: Vivien Didelot, linux-kernel, Kernel Hardening, netdev,
David S. Miller, Andrew Lunn, Kees Cook
In-Reply-To: <b948ef55-8b16-fee4-b22f-4c6d09fea0cf@gmail.com>
2018-03-13 21:06 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
> On 03/13/2018 12:58 PM, Vivien Didelot wrote:
>> Hi Salvatore,
>>
>> Salvatore Mesoraca <s.mesoraca16@gmail.com> writes:
>>
>>> dsa_switch's num_ports is currently fixed to DSA_MAX_PORTS. So we avoid
>>> 2 VLAs[1] by using DSA_MAX_PORTS instead of ds->num_ports.
>>>
>>> [1] https://lkml.org/lkml/2018/3/7/621
>>>
>>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>>
>> NAK.
>>
>> We are in the process to remove hardcoded limits such as DSA_MAX_PORTS
>> and DSA_MAX_SWITCHES, so we have to stick with ds->num_ports.
>
> Then this means that we need to allocate a bitmap from the heap, which
> sounds a bit superfluous and could theoretically fail... not sure which
> way is better, but bumping the size to DSA_MAX_PORTS definitively does
> help people working on enabling -Wvla.
Hi Florian,
Should I consider this patch still NAKed or not?
Should I resend the patch with some modifications?
Thank you,
Salvatore
^ permalink raw reply
* Re: [net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups
From: Willem de Bruijn @ 2018-05-05 10:06 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Network Development, Willem de Bruijn, David Miller
In-Reply-To: <20180504182537.5194.72775.stgit@localhost.localdomain>
On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> This patch set addresses a number of issues I found while sorting out
> enabling UDP GSO Segmentation support for ixgbe/ixgbevf. Specifically there
> were a number of issues related to the checksum and such that seemed to
> cause either minor irregularities or kernel panics in the case of the
> offload request being allowed to traverse between name spaces.
Were you able to traverse GSO packets between network namespace before
adding to NETIF_F_GSO_SOFTWARE? It does appear that veth includes
NETIF_F_GSO_ENCAP_ALL, which also allows GSO.
In either case, it should not be possible for GSO packets to arrive on a veth
device, as that can result in queuing the GSO packet to a recipient socket.
In this regard veth is like loopback and must exclude GSO support.
I'll take a look.
^ permalink raw reply
* Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation
From: Willem de Bruijn @ 2018-05-05 10:01 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Network Development, Willem de Bruijn, David Miller
In-Reply-To: <20180504182958.5194.62398.stgit@localhost.localdomain>
On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> This patch is meant to allow us to avoid having to recompute the checksum
> from scratch and have it passed as a parameter.
>
> Instead of taking that approach we can take advantage of the fact that the
> length that was used to compute the existing checksum is included in the
> UDP header. If we cancel that out by adding the value XOR with 0xFFFF we
> can then just add the new length in and fold that into the new result.
>
> I think this may be fixing a checksum bug in the original code as well
> since the checksum that was passed included the UDP header in the checksum
> computation, but then excluded it for the adjustment on the last frame. I
> believe this may have an effect on things in the cases where the two differ
> by bits that would result in things crossing the byte boundaries.
The replacement code, below, subtracts original payload size then adds
the new payload size. mss here excludes the udp header size.
> /* last packet can be partial gso_size */
> - if (!seg->next)
> - csum_replace2(&uh->check, htons(mss),
> - htons(seg->len - hdrlen - sizeof(*uh)));
^ permalink raw reply
* Re: [PATCH 7/8] rhashtable: add rhashtable_walk_prev()
From: Herbert Xu @ 2018-05-05 9:43 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel, Tom Herbert
In-Reply-To: <152540605441.18473.4087381584733882012.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> rhashtable_walk_prev() returns the object returned by
> the previous rhashtable_walk_next(), providing it is still in the
> table (or was during this grace period).
> This works even if rhashtable_walk_stop() and rhashtable_talk_start()
> have been called since the last rhashtable_walk_next().
>
> If there have been no calls to rhashtable_walk_next(), or if the
> object is gone from the table, then NULL is returned.
>
> This can usefully be used in a seq_file ->start() function.
> If the pos is the same as was returned by the last ->next() call,
> then rhashtable_walk_prev() can be used to re-establish the
> current location in the table. If it returns NULL, then
> rhashtable_walk_next() should be used.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
I will ack this if Tom is OK with replacing peek with it.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 6/8] rhashtable: further improve stability of rhashtable_walk
From: Herbert Xu @ 2018-05-05 9:42 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <152540605438.18473.4797800779538116583.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> If the sequence:
> obj = rhashtable_walk_next(iter);
> rhashtable_walk_stop(iter);
> rhashtable_remove_fast(ht, &obj->head, params);
> rhashtable_walk_start(iter);
>
> races with another thread inserting or removing
> an object on the same hash chain, a subsequent
> rhashtable_walk_next() is not guaranteed to get the "next"
> object. It is possible that an object could be
> repeated, or missed.
>
> This can be made more reliable by keeping the objects in a hash chain
> sorted by memory address. A subsequent rhashtable_walk_next()
> call can reliably find the correct position in the list, and thus
> find the 'next' object.
>
> It is not possible (certainly not so easy) to achieve this with an
> rhltable as keeping the hash chain in order is not so easy. When the
> first object with a given key is removed, it is replaced in the chain
> with the next object with the same key, and the address of that
> object may not be correctly ordered.
> No current user of rhltable_walk_enter() calls
> rhashtable_walk_start() more than once, so no current code
> could benefit from a more reliable walk of rhltables.
>
> This patch only attempts to improve walks for rhashtables.
> - a new object is always inserted after the last object with a
> smaller address, or at the start
> - when rhashtable_walk_start() is called, it records that 'p' is not
> 'safe', meaning that it cannot be dereferenced. The revalidation
> that was previously done here is moved to rhashtable_walk_next()
> - when rhashtable_walk_next() is called while p is not NULL and not
> safe, it walks the chain looking for the first object with an
> address greater than p and returns that. If there is none, it moves
> to the next hash chain.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
I'm a bit torn on this. On the hand this is definitely an improvement
over the status quo. On the other this does not work on rhltable and
we do have a way of fixing it for both rhashtable and rhltable.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 8/8] rhashtable: don't hold lock on first table throughout insertion.
From: Herbert Xu @ 2018-05-05 9:41 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <152540605444.18473.9591316658457316578.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> rhashtable_try_insert() currently hold a lock on the bucket in
> the first table, while also locking buckets in subsequent tables.
> This is unnecessary and looks like a hold-over from some earlier
> version of the implementation.
>
> As insert and remove always lock a bucket in each table in turn, and
> as insert only inserts in the final table, there cannot be any races
> that are not covered by simply locking a bucket in each table in turn.
>
> When an insert call reaches that last table it can be sure that there
> is no match entry in any other table as it has searched them all, and
> insertion never happens anywhere but in the last table. The fact that
> code tests for the existence of future_tbl while holding a lock on
> the relevant bucket ensures that two threads inserting the same key
> will make compatible decisions about which is the "last" table.
>
> This simplifies the code and allows the ->rehash field to be
> discarded.
> We still need a way to ensure that a dead bucket_table is never
> re-linked by rhashtable_walk_stop(). This can be achieved by
> setting the ->size to 1. This still allows lookup code to work (and
> correctly not find anything) but can never happen on an active bucket
> table (as the minimum size is 4).
>
> Signed-off-by: NeilBrown <neilb@suse.com>
I'm not convinced this is safe. There can be multiple tables
in existence. Unless you hold the lock on the first table, what
is to stop two parallel inserters each from inserting into their
"last" tables which may not be the same table?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 5/8] rhashtable: remove rhashtable_walk_peek()
From: Herbert Xu @ 2018-05-05 9:30 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel, Tom Herbert
In-Reply-To: <152540605435.18473.12657571579874985957.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> This function has a somewhat confused behavior that is not properly
> described by the documentation.
> Sometimes is returns the previous object, sometimes it returns the
> next one.
> Sometimes it changes the iterator, sometimes it doesn't.
>
> This function is not currently used and is not worth keeping, so
> remove it.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
I don't have a problem with this. But it would be good to hear
from Tom Herbert who added this.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 4/8] rhashtable: fix race in nested_table_alloc()
From: Herbert Xu @ 2018-05-05 9:29 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <152540605432.18473.11813271279255176724.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> If two threads run nested_table_alloc() at the same time
> they could both allocate a new table.
> Best case is that one of them will never be freed, leaking memory.
> Worst case is hat entry get stored there before it leaks,
> and the are lost from the table.
>
> So use cmpxchg to detect the race and free the unused table.
>
> Fixes: da20420f83ea ("rhashtable: Add nested tables")
> Cc: stable@vger.kernel.org # 4.11+
> Signed-off-by: NeilBrown <neilb@suse.com>
What about the spinlock that's meant to be held around this
operation?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/8] rhashtable: use cmpxchg() to protect ->future_tbl.
From: Herbert Xu @ 2018-05-05 9:27 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <152540605430.18473.11758878046224478232.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> Rather than borrowing one of the bucket locks to
> protect ->future_tbl updates, use cmpxchg().
> This gives more freedom to change how bucket locking
> is implemented.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
This looks nice.
> - spin_unlock_bh(old_tbl->locks);
> + rcu_assign_pointer(tmp, new_tbl);
Do we need this barrier since cmpxchg is supposed to provide memory
barrier semantics?
> + if (cmpxchg(&old_tbl->future_tbl, NULL, tmp) != NULL)
> + return -EEXIST;
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 2/8] rhashtable: remove nulls_base and related code.
From: Herbert Xu @ 2018-05-05 9:12 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <152540605427.18473.12277050439942480863.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> This "feature" is unused, undocumented, and untested and so
> doesn't really belong. If a use for the nulls marker
> is found, all this code would need to be reviewed to
> ensure it works as required. It would be just as easy to
> just add the code if/when it is needed instead.
>
> This patch actually fixes a bug too. The table resizing allows a
> table to grow to 2^31 buckets, but the hash is truncated to 27 bits -
> any growth beyond 2^27 is wasteful an ineffective.
>
> This patch result in NULLS_MARKER(0) being used for all chains,
> and leave the use of rht_is_a_null() to test for it.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
I disagree. This is a fundamental requirement for the use of
rhashtable in certain networking systems such as TCP/UDP. So
we know that there will be a use for this.
As to the bug fix, please separate it out of the patch and resubmit.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 1/8] rhashtable: silence RCU warning in rhashtable_test.
From: Herbert Xu @ 2018-05-05 9:10 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <152540605424.18473.310003836978239224.stgit@noble>
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote:
> print_ht in rhashtable_test calls rht_dereference() with neither
> RCU protection or the mutex. This triggers an RCU warning.
> So take the mutex to silence the warning.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
I don't think the mutex is actually needed but since we don't
have rht_dereference_raw and this is just test code:
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* BUG?: receiving on a packet socket with .sll_protocoll and bridging
From: Uwe Kleine-König @ 2018-05-05 8:57 UTC (permalink / raw)
To: netdev
Hello,
my eventual goal is to implement MRP and for that I started to program a
bit and stumbled over a problem I don't understand.
For testing purposes I created a veth device pair (veth0 veth1), open a
socket for each of the devices and send packets around between them. In
tcpdump a typical package looks as follows:
10:36:34.755208 ae:a9:da:50:48:db (oui Unknown) > 01:15:e4:00:00:01 (oui Unknown), ethertype Unknown (0x88e3), length 58:
0x0000: 0001 0212 8000 aea9 da50 48db 0000 0000 .........PH.....
0x0010: 0000 0589 40f2 6574 6800 0000 0000 0000 ....@.eth.......
0x0020: 0000 0100 0a80 3d38 4c5e 0000 ......=8L^..
The socket to receive these packages is opened using:
#define ETH_P_MRP 0x88e3
struct sockaddr_ll sa_ll = {
.sll_family = AF_PACKET,
.sll_protocol = htons(ETH_P_MRP),
.sll_ifindex = if_nametoindex("veth0")
};
fd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_MRP));
bind(fd, (struct sockaddr *)&sa_ll, sizeof(sa_ll));
So far everything works fine and I can receive the packets I send.
If now I add veth0 to a bridge (e.g.
ip link add br0 type bridge
ip link set dev veth0 master br0
) and continue to send on veth1 and receive on veth0 I don't receive
the packets any more. The other direction (veth0 sending, veth1
receiving) still works fine. Each of the following changes allow to
receive again:
a) take veth0 out of the bridge
b) bind(2) the receiving socket to br0 instead of veth0
c) use .sll_protocol = htons(ETH_P_ALL) for bind(2)
In the end only c) could be sensible (because I need to know the port
the packet entered the stack and that might well be bridged), but I
wonder why .sll_protocol = htons(ETH_P_MRP) seems to do the right thing
for an unbridged device but not for a bridged one.
Is this a bug or a feature I don't understand?
If someone wants to reproduce this locally, I can simplify my program
and provide it here. I tested this on a Debian 4.15 kernel (x86), but
also with the same symptoms on an arm64 with 4.16 and a dsa switch.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [net-next PATCH v2 7/8] udp: Do not copy destructor if one is not present
From: Willem de Bruijn @ 2018-05-05 8:45 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Network Development, Willem de Bruijn, David Miller
In-Reply-To: <20180504183110.5194.5449.stgit@localhost.localdomain>
On Fri, May 4, 2018 at 8:31 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> This patch makes it so that if a destructor is not present we avoid trying
> to update the skb socket or any reference counting that would be associated
> with the NULL socket and/or descriptor. By doing this we can support
> traffic coming from another namespace without any issues.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [net-next PATCH v2 5/8] udp: Partially unroll handling of first segment and last segment
From: Willem de Bruijn @ 2018-05-05 8:37 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Network Development, Willem de Bruijn, David Miller
In-Reply-To: <20180504183019.5194.47789.stgit@localhost.localdomain>
On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> This patch allows us to take care of unrolling the first segment and the
> last segment
Only the last segment needs to be unrolled, right?
> of the loop for processing the segmented skb. Part of the
> motivation for this is that it makes it easier to process the fact that the
> first fame and all of the frames in between should be mostly identical
> in terms of header data, and the last frame has differences in the length
> and partial checksum.
>
> In addition I am dropping the header length calculation since we don't
> really need it for anything but the last frame and it can be easily
> obtained by just pulling the data_len and offset of tail from the transport
> header.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>
> v2: New break-out patch based on one patch from earlier series
>
> net/ipv4/udp_offload.c | 35 ++++++++++++++++++++---------------
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 5c4bb8b9e83e..946d06d2aa0c 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -193,7 +193,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
> struct sock *sk = gso_skb->sk;
> unsigned int sum_truesize = 0;
> - unsigned int hdrlen;
> struct udphdr *uh;
> unsigned int mss;
> __sum16 check;
> @@ -206,7 +205,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> if (!pskb_may_pull(gso_skb, sizeof(*uh)))
> goto out;
>
> - hdrlen = gso_skb->data - skb_mac_header(gso_skb);
> skb_pull(gso_skb, sizeof(*uh));
>
> /* clear destructor to avoid skb_segment assigning it to tail */
> @@ -219,7 +217,8 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> return segs;
> }
>
> - uh = udp_hdr(segs);
> + seg = segs;
> + uh = udp_hdr(seg);
>
> /* compute checksum adjustment based on old length versus new */
> newlen = htons(sizeof(*uh) + mss);
> @@ -227,25 +226,31 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> ((__force u32)uh->len ^ 0xFFFF) +
> (__force u32)newlen));
>
> - for (seg = segs; seg; seg = seg->next) {
> - uh = udp_hdr(seg);
> + for (;;) {
> + seg->destructor = sock_wfree;
> + seg->sk = sk;
> + sum_truesize += seg->truesize;
>
> - /* last packet can be partial gso_size */
> - if (!seg->next) {
> - newlen = htons(seg->len - hdrlen);
> - check = ~csum_fold((__force __wsum)((__force u32)uh->check +
> - ((__force u32)uh->len ^ 0xFFFF) +
> - (__force u32)newlen));
> - }
> + if (!seg->next)
> + break;
Not critical, but I find replacing
for (seg = segs; seg; seg = seg->next) {
uh = udp_hdr(seg);
...
}
with
uh = udp_hdr(seg);
for (;;) {
...
if (!seg->next)
break;
uh = udp_hdr(seg);
}
much less easy to parse and it inflates patch size. How about just
- for (seg = segs; seg; seg = seg->next)
+ for( seg = segs; seg->next; seg = seg->next)
>
> uh->len = newlen;
> uh->check = check;
>
> - seg->destructor = sock_wfree;
> - seg->sk = sk;
> - sum_truesize += seg->truesize;
> + seg = seg->next;
> + uh = udp_hdr(seg);
> }
>
> + /* last packet can be partial gso_size, account for that in checksum */
> + newlen = htons(skb_tail_pointer(seg) - skb_transport_header(seg) +
> + seg->data_len);
> + check = ~csum_fold((__force __wsum)((__force u32)uh->check +
> + ((__force u32)uh->len ^ 0xFFFF) +
> + (__force u32)newlen));
> + uh->len = newlen;
> + uh->check = check;
> +
> + /* update refcount for the packet */
> refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
> out:
> return segs;
>
^ permalink raw reply
* Re: [net-next PATCH v2 1/8] udp: Record gso_segs when supporting UDP segmentation offload
From: Willem de Bruijn @ 2018-05-05 8:23 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Network Development, Willem de Bruijn, David Miller
In-Reply-To: <20180504182847.5194.27449.stgit@localhost.localdomain>
On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> We need to record the number of segments that will be generated when this
> frame is segmented. The expectation is that if gso_size is set then
> gso_segs is set as well. Without this some drivers such as ixgbe get
> confused if they attempt to offload this as they record 0 segments for the
> entire packet instead of the correct value.
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> net/ipv4/udp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index dd3102a37ef9..e07db83b311e 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -793,6 +793,8 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>
> skb_shinfo(skb)->gso_size = cork->gso_size;
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
> + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
> + cork->gso_size);
> goto csum_partial;
> }
Acked-by: Willem de Bruijn <willemb@google.com>
But we have to be careful that both UDP GSO and UFO packets can traverse
qdisc_pkt_len_init. This does seem to fix the UDP GSO case, as it adds the
header size to each segment. But that is odd, as the code precedes UDP
GSO, so must have been intended to estimate UFO size on the wire. Only
external sources can generate UFO. It seems like we need to not add
sizeof(struct udphdr) to hdrlen in the SKB_GSO_DODGY branch.
^ permalink raw reply
* Re: [net-next PATCH v2 3/8] udp: Do not pass MSS as parameter to GSO segmentation
From: Willem de Bruijn @ 2018-05-05 8:13 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexander Duyck, Network Development, Willem de Bruijn,
David Miller
In-Reply-To: <84200426-da0f-6ff4-8ae1-209e417f76c6@gmail.com>
On Fri, May 4, 2018 at 10:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/04/2018 11:29 AM, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> There is no point in passing MSS as a parameter for for the GSO
>> segmentation call as it is already available via the shared info for the
>> skb itself.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [net-next PATCH v2 2/8] udp: Verify that pulling UDP header in GSO segmentation doesn't fail
From: Willem de Bruijn @ 2018-05-05 8:12 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Network Development, Willem de Bruijn, David Miller
In-Reply-To: <20180504182857.5194.45504.stgit@localhost.localdomain>
On Fri, May 4, 2018 at 8:29 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> We should verify that we can pull the UDP header before we attempt to do
> so. Otherwise if this fails we have no way of knowing and GSO will not work
> correctly.
This already happened in the callers udp[46]_ufo_fragment
^ permalink raw reply
* Re: [RFC PATCH 2/2] net: mac808211: mac802154: use lockdep_assert_in_softirq() instead own warning
From: Ingo Molnar @ 2018-05-05 7:06 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Peter Zijlstra, linux-kernel, tglx, Ingo Molnar, David S. Miller,
Johannes Berg, Alexander Aring, Stefan Schmidt, netdev,
linux-wireless, linux-wpan, Anna-Maria Gleixner
In-Reply-To: <20180504190735.izmzibhb66gjb5wr@linutronix.de>
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2018-05-04 20:51:32 [+0200], Peter Zijlstra wrote:
> > On Fri, May 04, 2018 at 08:45:39PM +0200, Sebastian Andrzej Siewior wrote:
> > > On 2018-05-04 20:32:49 [+0200], Peter Zijlstra wrote:
> > > > On Fri, May 04, 2018 at 07:51:44PM +0200, Sebastian Andrzej Siewior wrote:
> > > > > From: Anna-Maria Gleixner <anna-maria@linutronix.de>
> > > > >
> > > > > The warning in ieee802154_rx() and ieee80211_rx_napi() is there to ensure
> > > > > the softirq context for the subsequent netif_receive_skb() call.
> > > >
> > > > That's not in fact what it does though; so while that might indeed be
> > > > the intent that's not what it does.
> > >
> > > It was introduced in commit d20ef63d3246 ("mac80211: document
> > > ieee80211_rx() context requirement"):
> > >
> > > mac80211: document ieee80211_rx() context requirement
> > >
> > > ieee80211_rx() must be called with softirqs disabled
> >
> > softirqs disabled, ack that is exactly what it checks.
> >
> > But afaict the assertion you introduced tests that we are _in_ softirq
> > context, which is not the same.
>
> indeed, now it clicked. Given what I wrote in the cover letter would you
> be in favour of (a proper) lockdep_assert_BH_disabled() or the cheaper
> local_bh_enable() (assuming the network folks don't mind the cheaper
> version)?
BTW., going by the hardirq variant nomenclature:
lockdep_assert_irqs_enabled();
... the proper name would not be lockdep_assert_BH_disabled(), but:
lockdep_assert_softirqs_disabled();
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH net] sctp: delay the authentication for the duplicated cookie-echo chunk
From: Xin Long @ 2018-05-05 7:01 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: network dev, linux-sctp, davem, Neil Horman
In-Reply-To: <20180504223337.GO5105@localhost.localdomain>
On Sat, May 5, 2018 at 6:33 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, May 04, 2018 at 05:05:10PM +0800, Xin Long wrote:
>> Now sctp only delays the authentication for the normal cookie-echo
>> chunk by setting chunk->auth_chunk in sctp_endpoint_bh_rcv(). But
>> for the duplicated one with auth, in sctp_assoc_bh_rcv(), it does
>> authentication first based on the old asoc, which will definitely
>> fail due to the different auth info in the old asoc.
>>
>> The duplicated cookie-echo chunk will create a new asoc with the
>> auth info from this chunk, and the authentication should also be
>> done with the new asoc's auth info for all of the collision 'A',
>> 'B' and 'D'. Otherwise, the duplicated cookie-echo chunk with auth
>> will never pass the authentication and create the new connection.
>>
>> This issue exists since very beginning, and this fix is to make
>> sctp_assoc_bh_rcv() follow the way sctp_assoc_bh_rcv() does for
> I guess you meant sctp_endpoint_bh_rcv here --^ right?
have posted v2, thanks.
>
> Other than this LGTM
>
>> the normal cookie-echo chunk to delay the authentication.
>>
>> While at it, remove the unused params from sctp_sf_authenticate()
>> and define sctp_auth_chunk_verify() used for all the places that
>> do the delayed authentication.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/sctp/associola.c | 30 ++++++++++++++++-
>> net/sctp/sm_statefuns.c | 86 ++++++++++++++++++++++++++-----------------------
>> 2 files changed, 75 insertions(+), 41 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 837806d..a47179d 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1024,8 +1024,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>> struct sctp_endpoint *ep;
>> struct sctp_chunk *chunk;
>> struct sctp_inq *inqueue;
>> - int state;
>> + int first_time = 1; /* is this the first time through the loop */
>> int error = 0;
>> + int state;
>>
>> /* The association should be held so we should be safe. */
>> ep = asoc->ep;
>> @@ -1036,6 +1037,30 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>> state = asoc->state;
>> subtype = SCTP_ST_CHUNK(chunk->chunk_hdr->type);
>>
>> + /* If the first chunk in the packet is AUTH, do special
>> + * processing specified in Section 6.3 of SCTP-AUTH spec
>> + */
>> + if (first_time && subtype.chunk == SCTP_CID_AUTH) {
>> + struct sctp_chunkhdr *next_hdr;
>> +
>> + next_hdr = sctp_inq_peek(inqueue);
>> + if (!next_hdr)
>> + goto normal;
>> +
>> + /* If the next chunk is COOKIE-ECHO, skip the AUTH
>> + * chunk while saving a pointer to it so we can do
>> + * Authentication later (during cookie-echo
>> + * processing).
>> + */
>> + if (next_hdr->type == SCTP_CID_COOKIE_ECHO) {
>> + chunk->auth_chunk = skb_clone(chunk->skb,
>> + GFP_ATOMIC);
>> + chunk->auth = 1;
>> + continue;
>> + }
>> + }
>> +
>> +normal:
>> /* SCTP-AUTH, Section 6.3:
>> * The receiver has a list of chunk types which it expects
>> * to be received only after an AUTH-chunk. This list has
>> @@ -1074,6 +1099,9 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
>> /* If there is an error on chunk, discard this packet. */
>> if (error && chunk)
>> chunk->pdiscard = 1;
>> +
>> + if (first_time)
>> + first_time = 0;
>> }
>> sctp_association_put(asoc);
>> }
>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>> index 28c070e..c9ae340 100644
>> --- a/net/sctp/sm_statefuns.c
>> +++ b/net/sctp/sm_statefuns.c
>> @@ -153,10 +153,7 @@ static enum sctp_disposition sctp_sf_violation_chunk(
>> struct sctp_cmd_seq *commands);
>>
>> static enum sctp_ierror sctp_sf_authenticate(
>> - struct net *net,
>> - const struct sctp_endpoint *ep,
>> const struct sctp_association *asoc,
>> - const union sctp_subtype type,
>> struct sctp_chunk *chunk);
>>
>> static enum sctp_disposition __sctp_sf_do_9_1_abort(
>> @@ -626,6 +623,38 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
>> return SCTP_DISPOSITION_CONSUME;
>> }
>>
>> +static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
>> + const struct sctp_association *asoc)
>> +{
>> + struct sctp_chunk auth;
>> +
>> + if (!chunk->auth_chunk)
>> + return true;
>> +
>> + /* SCTP-AUTH: auth_chunk pointer is only set when the cookie-echo
>> + * is supposed to be authenticated and we have to do delayed
>> + * authentication. We've just recreated the association using
>> + * the information in the cookie and now it's much easier to
>> + * do the authentication.
>> + */
>> +
>> + /* Make sure that we and the peer are AUTH capable */
>> + if (!net->sctp.auth_enable || !asoc->peer.auth_capable)
>> + return false;
>> +
>> + /* set-up our fake chunk so that we can process it */
>> + auth.skb = chunk->auth_chunk;
>> + auth.asoc = chunk->asoc;
>> + auth.sctp_hdr = chunk->sctp_hdr;
>> + auth.chunk_hdr = (struct sctp_chunkhdr *)
>> + skb_push(chunk->auth_chunk,
>> + sizeof(struct sctp_chunkhdr));
>> + skb_pull(chunk->auth_chunk, sizeof(struct sctp_chunkhdr));
>> + auth.transport = chunk->transport;
>> +
>> + return sctp_sf_authenticate(asoc, &auth) == SCTP_IERROR_NO_ERROR;
>> +}
>> +
>> /*
>> * Respond to a normal COOKIE ECHO chunk.
>> * We are the side that is being asked for an association.
>> @@ -763,37 +792,9 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
>> if (error)
>> goto nomem_init;
>>
>> - /* SCTP-AUTH: auth_chunk pointer is only set when the cookie-echo
>> - * is supposed to be authenticated and we have to do delayed
>> - * authentication. We've just recreated the association using
>> - * the information in the cookie and now it's much easier to
>> - * do the authentication.
>> - */
>> - if (chunk->auth_chunk) {
>> - struct sctp_chunk auth;
>> - enum sctp_ierror ret;
>> -
>> - /* Make sure that we and the peer are AUTH capable */
>> - if (!net->sctp.auth_enable || !new_asoc->peer.auth_capable) {
>> - sctp_association_free(new_asoc);
>> - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>> - }
>> -
>> - /* set-up our fake chunk so that we can process it */
>> - auth.skb = chunk->auth_chunk;
>> - auth.asoc = chunk->asoc;
>> - auth.sctp_hdr = chunk->sctp_hdr;
>> - auth.chunk_hdr = (struct sctp_chunkhdr *)
>> - skb_push(chunk->auth_chunk,
>> - sizeof(struct sctp_chunkhdr));
>> - skb_pull(chunk->auth_chunk, sizeof(struct sctp_chunkhdr));
>> - auth.transport = chunk->transport;
>> -
>> - ret = sctp_sf_authenticate(net, ep, new_asoc, type, &auth);
>> - if (ret != SCTP_IERROR_NO_ERROR) {
>> - sctp_association_free(new_asoc);
>> - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>> - }
>> + if (!sctp_auth_chunk_verify(net, chunk, new_asoc)) {
>> + sctp_association_free(new_asoc);
>> + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
>> }
>>
>> repl = sctp_make_cookie_ack(new_asoc, chunk);
>> @@ -1797,13 +1798,15 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
>> if (sctp_auth_asoc_init_active_key(new_asoc, GFP_ATOMIC))
>> goto nomem;
>>
>> + if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
>> + return SCTP_DISPOSITION_DISCARD;
>> +
>> /* Make sure no new addresses are being added during the
>> * restart. Though this is a pretty complicated attack
>> * since you'd have to get inside the cookie.
>> */
>> - if (!sctp_sf_check_restart_addrs(new_asoc, asoc, chunk, commands)) {
>> + if (!sctp_sf_check_restart_addrs(new_asoc, asoc, chunk, commands))
>> return SCTP_DISPOSITION_CONSUME;
>> - }
>>
>> /* If the endpoint is in the SHUTDOWN-ACK-SENT state and recognizes
>> * the peer has restarted (Action A), it MUST NOT setup a new
>> @@ -1912,6 +1915,9 @@ static enum sctp_disposition sctp_sf_do_dupcook_b(
>> if (sctp_auth_asoc_init_active_key(new_asoc, GFP_ATOMIC))
>> goto nomem;
>>
>> + if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
>> + return SCTP_DISPOSITION_DISCARD;
>> +
>> /* Update the content of current association. */
>> sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
>> sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>> @@ -2009,6 +2015,9 @@ static enum sctp_disposition sctp_sf_do_dupcook_d(
>> * a COOKIE ACK.
>> */
>>
>> + if (!sctp_auth_chunk_verify(net, chunk, asoc))
>> + return SCTP_DISPOSITION_DISCARD;
>> +
>> /* Don't accidentally move back into established state. */
>> if (asoc->state < SCTP_STATE_ESTABLISHED) {
>> sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
>> @@ -4171,10 +4180,7 @@ enum sctp_disposition sctp_sf_eat_fwd_tsn_fast(
>> * The return value is the disposition of the chunk.
>> */
>> static enum sctp_ierror sctp_sf_authenticate(
>> - struct net *net,
>> - const struct sctp_endpoint *ep,
>> const struct sctp_association *asoc,
>> - const union sctp_subtype type,
>> struct sctp_chunk *chunk)
>> {
>> struct sctp_shared_key *sh_key = NULL;
>> @@ -4275,7 +4281,7 @@ enum sctp_disposition sctp_sf_eat_auth(struct net *net,
>> commands);
>>
>> auth_hdr = (struct sctp_authhdr *)chunk->skb->data;
>> - error = sctp_sf_authenticate(net, ep, asoc, type, chunk);
>> + error = sctp_sf_authenticate(asoc, chunk);
>> switch (error) {
>> case SCTP_IERROR_AUTH_BAD_HMAC:
>> /* Generate the ERROR chunk and discard the rest
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox