* Re: [PATCH] netlink: fix memory leak of dump
From: David Miller @ 2018-07-22 17:07 UTC (permalink / raw)
To: fw
Cc: cscnull, pablo, kadlec, johannes.berg, Jason, ktkhai, lucien.xin,
xiyou.wangcong, dsahern, netfilter-devel, tom, netdev,
linux-kernel
In-Reply-To: <20180722163925.gdfkndldatsoae6x@breakpoint.cc>
From: Florian Westphal <fw@strlen.de>
Date: Sun, 22 Jul 2018 18:39:25 +0200
> 3. change meaning of ->done() so its always called once ->start()
> was invoked (and returned 0), this requires audit of all
> places that provide .done to make sure they won't trip.
>
> 3) seems to be what Tom intended when he added .start, so probably
> best to investigate that first.
Hmmm...
Any time ->start() succeeds, we set cb_running to true.
>From that point forward, ->done() will be called at some point at all
of the locations that check if cb_running is true and set it to false.
This may be deferred all the way to socket destruction, but it will
eventually happens.
Nothing sets cb_running to false without invoking the ->done()
callback.
Just because the individual dump invocations don't call ->done() does
not mean it will not eventually happen. The dump callbacks, and thus
the state data, is live across multiple rounds of recvmsg() calls by
the user and that is as-designed.
^ permalink raw reply
* Re: [PATCH net-next] bonding: don't cast const buf in sysfs store
From: David Miller @ 2018-07-22 17:09 UTC (permalink / raw)
To: nikolay; +Cc: netdev, j.vosburgh, vfalico, andy, roopa
In-Reply-To: <20180722083731.31888-1-nikolay@cumulusnetworks.com>
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Sun, 22 Jul 2018 11:37:31 +0300
> As was recently discussed [1], let's avoid casting the const buf in
> bonding_sysfs_store_option and use kstrndup/kfree instead.
>
> [1] http://lists.openwall.net/netdev/2018/07/22/25
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Applied, thanks Nikolay.
^ permalink raw reply
* Re: [PATCH] netlink: fix memory leak of dump
From: Florian Westphal @ 2018-07-22 18:09 UTC (permalink / raw)
To: David Miller
Cc: fw, cscnull, pablo, kadlec, johannes.berg, Jason, ktkhai,
lucien.xin, xiyou.wangcong, dsahern, netfilter-devel, tom, netdev,
linux-kernel
In-Reply-To: <20180722.100755.19840167505550163.davem@davemloft.net>
David Miller <davem@davemloft.net> wrote:
> From: Florian Westphal <fw@strlen.de>
> Date: Sun, 22 Jul 2018 18:39:25 +0200
>
> > 3. change meaning of ->done() so its always called once ->start()
> > was invoked (and returned 0), this requires audit of all
> > places that provide .done to make sure they won't trip.
> >
> > 3) seems to be what Tom intended when he added .start, so probably
> > best to investigate that first.
>
> Hmmm...
>
> Any time ->start() succeeds, we set cb_running to true.
Right.
> From that point forward, ->done() will be called at some point at all
> of the locations that check if cb_running is true and set it to false.
Also right, thanks for pointing this out, I missed fact that netlink
core restarts a dump after this.
So 3) is already true which means we should try to see if we can move
all dump-related extra magic into ->start().
Shaochun, can you see if this is possible?
Something along these lines (totally untested), which makes this
a netfilter fix:
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])
return filter;
}
+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+ const struct nlattr * const *nla = cb->data;
+ struct nft_obj_filter *filter = NULL;
+
+ if (nla[NFTA_OBJ_TABLE] ||
+ nla[NFTA_OBJ_TYPE]) {
+ filter = nft_obj_filter_alloc(nla);
+ if (IS_ERR(filter))
+ return -ENOMEM;
+ }
+
+ cb->data = filter;
+ return 0;
+}
+
/* called with rcu_read_lock held */
static int nf_tables_getobj(struct net *net, struct sock *nlsk,
struct sk_buff *skb, const struct nlmsghdr *nlh,
@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,
if (nlh->nlmsg_flags & NLM_F_DUMP) {
struct netlink_dump_control c = {
+ .start = nf_tables_dump_obj_start,
.dump = nf_tables_dump_obj,
.done = nf_tables_dump_obj_done,
.module = THIS_MODULE,
+ .data = (void *)nla,
};
- if (nla[NFTA_OBJ_TABLE] ||
- nla[NFTA_OBJ_TYPE]) {
- struct nft_obj_filter *filter;
-
- filter = nft_obj_filter_alloc(nla);
- if (IS_ERR(filter))
- return -ENOMEM;
-
- c.data = filter;
- }
return nft_netlink_dump_start_rcu(nlsk, skb, nlh, &c);
}
^ permalink raw reply
* Re: [PATCH net-next] xen-netfront: fix queue name setting
From: David Miller @ 2018-07-22 17:27 UTC (permalink / raw)
To: vkuznets
Cc: netdev, xen-devel, linux-kernel, boris.ostrovsky, jgross,
ross.lagerwall
In-Reply-To: <20180720163359.28187-1-vkuznets@redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Fri, 20 Jul 2018 18:33:59 +0200
> Commit f599c64fdf7d ("xen-netfront: Fix race between device setup and
> open") changed the initialization order: xennet_create_queues() now
> happens before we do register_netdev() so using netdev->name in
> xennet_init_queue() is incorrect, we end up with the following in
> /proc/interrupts:
>
> 60: 139 0 xen-dyn -event eth%d-q0-tx
> 61: 265 0 xen-dyn -event eth%d-q0-rx
> 62: 234 0 xen-dyn -event eth%d-q1-tx
> 63: 1 0 xen-dyn -event eth%d-q1-rx
>
> and this looks ugly. Actually, using early netdev name (even when it's
> already set) is also not ideal: nowadays we tend to rename eth devices
> and queue name may end up not corresponding to the netdev name.
>
> Use nodename from xenbus device for queue naming: this can't change in VM's
> lifetime. Now /proc/interrupts looks like
>
> 62: 202 0 xen-dyn -event device/vif/0-q0-tx
> 63: 317 0 xen-dyn -event device/vif/0-q0-rx
> 64: 262 0 xen-dyn -event device/vif/0-q1-tx
> 65: 17 0 xen-dyn -event device/vif/0-q1-rx
>
> Fixes: f599c64fdf7d ("xen-netfront: Fix race between device setup and open")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Patch applied, thank you.
^ permalink raw reply
* Re: [PATCH net] atl1c: reserve min skb headroom
From: David Miller @ 2018-07-22 17:29 UTC (permalink / raw)
To: fw; +Cc: netdev, eric.dumazet
In-Reply-To: <20180720173057.11365-1-fw@strlen.de>
From: Florian Westphal <fw@strlen.de>
Date: Fri, 20 Jul 2018 19:30:57 +0200
> Got crash report with following backtrace:
> BUG: unable to handle kernel paging request at ffff8801869daffe
> RIP: 0010:[<ffffffff816429c4>] [<ffffffff816429c4>] ip6_finish_output2+0x394/0x4c0
> RSP: 0018:ffff880186c83a98 EFLAGS: 00010283
> RAX: ffff8801869db00e ...
> [<ffffffff81644cdc>] ip6_finish_output+0x8c/0xf0
> [<ffffffff81644d97>] ip6_output+0x57/0x100
> [<ffffffff81643dc9>] ip6_forward+0x4b9/0x840
> [<ffffffff81645566>] ip6_rcv_finish+0x66/0xc0
> [<ffffffff81645db9>] ipv6_rcv+0x319/0x530
> [<ffffffff815892ac>] netif_receive_skb+0x1c/0x70
> [<ffffffffc0060bec>] atl1c_clean+0x1ec/0x310 [atl1c]
> ...
>
> The bad access is in neigh_hh_output(), at skb->data - 16 (HH_DATA_MOD).
> atl1c driver provided skb with no headroom, so 14 bytes (ethernet
> header) got pulled, but then 16 are copied.
>
> Reserve NET_SKB_PAD bytes headroom, like netdev_alloc_skb().
>
> Compile tested only; I lack hardware.
>
> Fixes: 7b7017642199 ("atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring")
> Signed-off-by: Florian Westphal <fw@strlen.de>
Ancient bug :-/
Applied and queued up for -stable, thanks Florian.
^ permalink raw reply
* Re: [PATCH net 0/4] vxlan: fix default fdb entry user-space notify ordering/race
From: David Miller @ 2018-07-22 17:54 UTC (permalink / raw)
To: roopa; +Cc: netdev
In-Reply-To: <1532118064-45513-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Fri, 20 Jul 2018 13:21:00 -0700
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Problem:
> In vxlan_newlink, a default fdb entry is added before register_netdev.
> The default fdb creation function notifies user-space of the
> fdb entry on the vxlan device which user-space does not know about yet.
> (RTM_NEWNEIGH goes before RTM_NEWLINK for the same ifindex).
>
> This series fixes the user-space netlink notification ordering issue
> with the following changes:
> - decouple fdb notify from fdb create.
> - Move fdb notify after register_netdev.
> - modify rtnl_configure_link to allow configuring a link early.
> - Call rtnl_configure_link in vxlan newlink handler to notify
> userspace about the newlink before fdb notify and
> hence avoiding the user-space race.
>
> Fixes: afbd8bae9c79 ("vxlan: add implicit fdb entry for default destination")
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
Series applied and queued up for -stable, thanks Roopa.
^ permalink raw reply
* Re: [PATCH net] nfp: flower: ensure dead neighbour entries are not offloaded
From: David Miller @ 2018-07-22 17:56 UTC (permalink / raw)
To: jakub.kicinski; +Cc: oss-drivers, netdev, john.hurley
In-Reply-To: <20180721040754.22577-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 20 Jul 2018 21:07:54 -0700
> From: John Hurley <john.hurley@netronome.com>
>
> Previously only the neighbour state was checked to decide if an offloaded
> entry should be removed. However, there can be situations when the entry
> is dead but still marked as valid. This can lead to dead entries not
> being removed from fw tables or even incorrect data being added.
>
> Check the entry dead bit before deciding if it should be added to or
> removed from fw neighbour tables.
>
> Fixes: 8e6a9046b66a ("nfp: flower vxlan neighbour offload")
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Patch applied and queued up for -stable, thanks Jakub.
^ permalink raw reply
* Re: [PATCH net-next 1/2] nfp: bring back support for offloading shared blocks
From: David Miller @ 2018-07-22 17:59 UTC (permalink / raw)
To: jakub.kicinski; +Cc: oss-drivers, netdev
In-Reply-To: <20180721041439.23358-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 20 Jul 2018 21:14:38 -0700
> Now that we have offload replay infrastructure added by
> commit 326367427cc0 ("net: sched: call reoffload op on block callback reg")
> and flows are guaranteed to be removed correctly, we can revert
> commit 951a8ee6def3 ("nfp: reject binding to shared blocks").
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: John Hurley <john.hurley@netronome.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/2] nfp: avoid buffer leak when FW communication fails
From: David Miller @ 2018-07-22 17:59 UTC (permalink / raw)
To: jakub.kicinski; +Cc: oss-drivers, netdev
In-Reply-To: <20180721041439.23358-2-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 20 Jul 2018 21:14:39 -0700
> After device is stopped we reset the rings by moving all free buffers
> to positions [0, cnt - 2], and clear the position cnt - 1 in the ring.
> We then proceed to clear the read/write pointers. This means that if
> we try to reset the ring again the code will assume that the next to
> fill buffer is at position 0 and swap it with cnt - 1. Since we
> previously cleared position cnt - 1 it will lead to leaking the first
> buffer and leaving ring in a bad state.
>
> This scenario can only happen if FW communication fails, in which case
> the ring will never be used again, so the fact it's in a bad state will
> not be noticed. Buffer leak is the only problem. Don't try to move
> buffers in the ring if the read/write pointers indicate the ring was
> never used or have already been reset.
>
> nfp_net_clear_config_and_disable() is now fully idempotent.
>
> Found by code inspection, FW communication failures are very rare,
> and reconfiguring a live device is not common either, so it's unlikely
> anyone has ever noticed the leak.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Applied.
> This is arguably net material but IMHO the risk of me missing something
> this could break is higher than the error actually occurring, and a
> page leak on a FW communication error doesn't seem like it's worth
> it at -rc6 time.. I'm happy to respin if I'm wrong!
Agreed, net-next is more appropriate for this.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: phy: add helper phy_polling_mode
From: David Miller @ 2018-07-22 18:11 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, netdev
In-Reply-To: <18b53769-e444-663d-650d-5887b3ae53ad@gmail.com>
I think you can combine these two patches into one.
Thank you.
^ permalink raw reply
* Re: [PATCH] net: prevent ISA drivers from building on PPC32
From: David Miller @ 2018-07-22 18:13 UTC (permalink / raw)
To: rdunlap; +Cc: linuxppc-dev, mpe, netdev
In-Reply-To: <b7e66624-a077-048a-d8f5-52a3722157df@infradead.org>
From: Randy Dunlap <rdunlap@infradead.org>
Date: Sat, 21 Jul 2018 12:59:25 -0700
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Prevent drivers from building on PPC32 if they use isa_bus_to_virt(),
> isa_virt_to_bus(), or isa_page_to_bus(), which are not available and
> thus cause build errors.
...
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Applied, thanks Randy.
^ permalink raw reply
* Hello Beautiful
From: Jack @ 2018-07-22 17:29 UTC (permalink / raw)
Hi Dear, my name is Jack and i am seeking for a relationship in which i will feel loved after a series of failed relationships.
I am hoping that you would be interested and we could possibly get to know each other more if you do not mind. I am open to answering questions from you as i think my approach is a little inappropriate. Hope to hear back from you.
Jack.
^ permalink raw reply
* Re: [PATCH] net: dsa: mv88e6xxx: fix races between lock and irq freeing
From: Uwe Kleine-König @ 2018-07-22 19:00 UTC (permalink / raw)
To: David Miller; +Cc: andrew, vivien.didelot, f.fainelli, netdev, kernel
In-Reply-To: <20180721.224409.437815752648454744.davem@davemloft.net>
Hello,
On Sat, Jul 21, 2018 at 10:44:09PM -0700, David Miller wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date: Fri, 20 Jul 2018 11:53:15 +0200
>
> > free_irq() waits until all handlers for this IRQ have completed. As the
> > relevant handler (mv88e6xxx_g1_irq_thread_fn()) takes the chip's reg_lock
> > it might never return if the thread calling free_irq() holds this lock.
> >
> > For the same reason kthread_cancel_delayed_work_sync() in the polling case
> > must not hold this lock.
> >
> > Also first free the irq (or stop the worker respectively) such that
> > mv88e6xxx_g1_irq_thread_work() isn't called any more before the irq
> > mappings are dropped in mv88e6xxx_g1_irq_free_common() to prevent the
> > worker thread to call handle_nested_irq(0) which results in a NULL-pointer
> > exception.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Looks good.
>
> Note than the IRQ domain unmapping will do a synchronize_irq() which
> should cause the same deadlock as free_irq() will with the reg_lock
> held.
Do you think that there is still a problem? When free_irq() for the
external visible irq returns the muxed irqs should be all gone, too, so
this should not trigger, should it?
> Note also that g2 IRQ freeing gets the ordering right, and doesn't need
> a lock because it doesn't program any registers when tearing down it's
> IRQ.
Yes.
> Applied and queued up for -stable, thanks.
Fine, thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 02/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-07-22 20:55 UTC (permalink / raw)
To: Steve Grubb
Cc: simo, carlos, linux-api, containers, linux-kernel, Eric Paris,
dhowells, linux-audit, ebiederm, luto, netdev, linux-fsdevel,
cgroups, serge, viro
In-Reply-To: <5467262.rd0RIe6TW9@x2>
On 2018-07-22 09:32, Steve Grubb wrote:
> On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > + * audit_log_contid - report container info
> > > > + * @tsk: task to be recorded
> > > > + * @context: task or local context for record
> > > > + * @op: contid string description
> > > > + */
> > > > +int audit_log_contid(struct task_struct *tsk,
> > > > + struct audit_context *context, char *op)
> > > > +{
> > > > + struct audit_buffer *ab;
> > > > +
> > > > + if (!audit_contid_set(tsk))
> > > > + return 0;
> > > > + /* Generate AUDIT_CONTAINER record with container ID */
> > > > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > + if (!ab)
> > > > + return -ENOMEM;
> > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > + op, audit_get_contid(tsk));
> > >
> > > Can you explain your reason for including an "op" field in this record
> > > type? I've been looking at the rest of the patches in this patchset
> > > and it seems to be used more as an indicator of the record's
> > > generating context rather than any sort of audit container ID
> > > operation.
> >
> > "action" might work, but that's netfilter and numeric... "kind"?
> > Nothing else really seems to fit from a field name, type or lack of
> > searchability perspective.
> >
> > Steve, do you have an opinion?
>
> We only have 1 sample event where we have op=task. What are the other
> possible values?
For the AUDIT_CONTAINER record we have op= "task", "target" (from the
ptrace and signals patch), "tty".
For the AUDIT_CONTAINER_ID record we have "op=set".
> -Steve
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH] net: dsa: mv88e6xxx: fix races between lock and irq freeing
From: David Miller @ 2018-07-22 20:04 UTC (permalink / raw)
To: u.kleine-koenig; +Cc: andrew, vivien.didelot, f.fainelli, netdev, kernel
In-Reply-To: <20180722190035.ehav25wwwtz3fxfm@pengutronix.de>
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Sun, 22 Jul 2018 21:00:35 +0200
> On Sat, Jul 21, 2018 at 10:44:09PM -0700, David Miller wrote:
>> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Date: Fri, 20 Jul 2018 11:53:15 +0200
>>
>> > free_irq() waits until all handlers for this IRQ have completed. As the
>> > relevant handler (mv88e6xxx_g1_irq_thread_fn()) takes the chip's reg_lock
>> > it might never return if the thread calling free_irq() holds this lock.
>> >
>> > For the same reason kthread_cancel_delayed_work_sync() in the polling case
>> > must not hold this lock.
>> >
>> > Also first free the irq (or stop the worker respectively) such that
>> > mv88e6xxx_g1_irq_thread_work() isn't called any more before the irq
>> > mappings are dropped in mv88e6xxx_g1_irq_free_common() to prevent the
>> > worker thread to call handle_nested_irq(0) which results in a NULL-pointer
>> > exception.
>> >
>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>
>> Looks good.
>>
>> Note than the IRQ domain unmapping will do a synchronize_irq() which
>> should cause the same deadlock as free_irq() will with the reg_lock
>> held.
>
> Do you think that there is still a problem? When free_irq() for the
> external visible irq returns the muxed irqs should be all gone, too, so
> this should not trigger, should it?
It shouldn't be a problem after your changes.
I'm just saying that I'm surprised that, in the original code, you see
the deadlock in free_irq(), since the synchronize_irq() done by the
IRQ domain code should have happened first.
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 02/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-07-22 21:03 UTC (permalink / raw)
To: Steve Grubb
Cc: simo, carlos, linux-api, containers, linux-kernel, Eric Paris,
dhowells, linux-audit, ebiederm, luto, netdev, linux-fsdevel,
cgroups, serge, viro
In-Reply-To: <20180722205510.eh2n7bcd52454dwj@madcap2.tricolour.ca>
On 2018-07-22 16:55, Richard Guy Briggs wrote:
> On 2018-07-22 09:32, Steve Grubb wrote:
> > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > + * audit_log_contid - report container info
> > > > > + * @tsk: task to be recorded
> > > > > + * @context: task or local context for record
> > > > > + * @op: contid string description
> > > > > + */
> > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > + struct audit_context *context, char *op)
> > > > > +{
> > > > > + struct audit_buffer *ab;
> > > > > +
> > > > > + if (!audit_contid_set(tsk))
> > > > > + return 0;
> > > > > + /* Generate AUDIT_CONTAINER record with container ID */
> > > > > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > > + if (!ab)
> > > > > + return -ENOMEM;
> > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > + op, audit_get_contid(tsk));
> > > >
> > > > Can you explain your reason for including an "op" field in this record
> > > > type? I've been looking at the rest of the patches in this patchset
> > > > and it seems to be used more as an indicator of the record's
> > > > generating context rather than any sort of audit container ID
> > > > operation.
> > >
> > > "action" might work, but that's netfilter and numeric... "kind"?
> > > Nothing else really seems to fit from a field name, type or lack of
> > > searchability perspective.
> > >
> > > Steve, do you have an opinion?
> >
> > We only have 1 sample event where we have op=task. What are the other
> > possible values?
>
> For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> ptrace and signals patch), "tty".
Sorry, pressed "send" too quickly. Also "aux0x%x" (also from the
ptrace/signals patch), "net%u" (from the AUDIT_NETFILTER_PKT patch).
> For the AUDIT_CONTAINER_ID record we have "op=set".
>
> > -Steve
>
> - RGB
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH] net: dsa: mv88e6xxx: fix races between lock and irq freeing
From: Uwe Kleine-König @ 2018-07-22 20:38 UTC (permalink / raw)
To: David Miller; +Cc: andrew, vivien.didelot, f.fainelli, netdev, kernel
In-Reply-To: <20180722.130411.1782941097108136413.davem@davemloft.net>
On Sun, Jul 22, 2018 at 01:04:11PM -0700, David Miller wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Date: Sun, 22 Jul 2018 21:00:35 +0200
>
> > On Sat, Jul 21, 2018 at 10:44:09PM -0700, David Miller wrote:
> >> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> Date: Fri, 20 Jul 2018 11:53:15 +0200
> >>
> >> > free_irq() waits until all handlers for this IRQ have completed. As the
> >> > relevant handler (mv88e6xxx_g1_irq_thread_fn()) takes the chip's reg_lock
> >> > it might never return if the thread calling free_irq() holds this lock.
> >> >
> >> > For the same reason kthread_cancel_delayed_work_sync() in the polling case
> >> > must not hold this lock.
> >> >
> >> > Also first free the irq (or stop the worker respectively) such that
> >> > mv88e6xxx_g1_irq_thread_work() isn't called any more before the irq
> >> > mappings are dropped in mv88e6xxx_g1_irq_free_common() to prevent the
> >> > worker thread to call handle_nested_irq(0) which results in a NULL-pointer
> >> > exception.
> >> >
> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >>
> >> Looks good.
> >>
> >> Note than the IRQ domain unmapping will do a synchronize_irq() which
> >> should cause the same deadlock as free_irq() will with the reg_lock
> >> held.
> >
> > Do you think that there is still a problem? When free_irq() for the
> > external visible irq returns the muxed irqs should be all gone, too, so
> > this should not trigger, should it?
>
> It shouldn't be a problem after your changes.
>
> I'm just saying that I'm surprised that, in the original code, you see
> the deadlock in free_irq(), since the synchronize_irq() done by the
> IRQ domain code should have happened first.
ah, I see. This didn't happen because I added an msleep to
mv88e6xxx_g1_irq_thread_work() before the lock it taken to widen the
race window for a different problem. So the sub-irqs were not active
when mv88e6xxx_g1_irq_free() run, only the mux-irq was. When
irq_dispose_mapping() is called for the sub-irq there is no problem as
this results in synchronize_irq() for the sub-irq, not the mux-irq.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v2] net: ethernet: freescale: Use generic CRC32 implementation
From: David Miller @ 2018-07-22 21:39 UTC (permalink / raw)
To: krzk; +Cc: fugang.duan, netdev, linux-kernel, ebiggers3
In-Reply-To: <20180722142636.31525-1-krzk@kernel.org>
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: Sun, 22 Jul 2018 16:26:36 +0200
> + crc = crc32_le(~0, ha->addr, ndev->addr_len);
This is "ether_crc_le(ha->addr, ndev->addr_len)" :-)
Please use it, thanks!
^ permalink raw reply
* Re: [PATCH 2/5] rhashtable: don't hold lock on first table throughout insertion.
From: Paul E. McKenney @ 2018-07-22 21:54 UTC (permalink / raw)
To: NeilBrown; +Cc: Herbert Xu, Thomas Graf, netdev, linux-kernel
In-Reply-To: <87muulqq8q.fsf@notabene.neil.brown.name>
On Sat, Jul 21, 2018 at 12:25:41PM +1000, NeilBrown wrote:
> On Fri, Jul 20 2018, Paul E. McKenney wrote:
>
> > On Fri, Jul 20, 2018 at 03:54:09PM +0800, Herbert Xu wrote:
> >> On Fri, Jul 06, 2018 at 05:22:30PM +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
> >> > calling call_rcu() inside the locked region, and checking
> >> > ->rcu.func in rhashtable_walk_stop(). If it is not NULL, then
> >> > the bucket table is empty and dead.
> >> >
> >> > Signed-off-by: NeilBrown <neilb@suse.com>
> >>
> >> ...
> >>
> >> > @@ -339,13 +338,16 @@ static int rhashtable_rehash_table(struct rhashtable *ht)
> >> > spin_lock(&ht->lock);
> >> > list_for_each_entry(walker, &old_tbl->walkers, list)
> >> > walker->tbl = NULL;
> >> > - spin_unlock(&ht->lock);
> >> >
> >> > /* Wait for readers. All new readers will see the new
> >> > * table, and thus no references to the old table will
> >> > * remain.
> >> > + * We do this inside the locked region so that
> >> > + * rhashtable_walk_stop() can check ->rcu.func and know
> >> > + * not to re-link the table.
> >> > */
> >> > call_rcu(&old_tbl->rcu, bucket_table_free_rcu);
> >> > + spin_unlock(&ht->lock);
> >> >
> >> > return rht_dereference(new_tbl->future_tbl, ht) ? -EAGAIN : 0;
> >> > }
> >>
> >> ...
> >>
> >> > @@ -964,7 +942,7 @@ void rhashtable_walk_stop(struct rhashtable_iter *iter)
> >> > ht = iter->ht;
> >> >
> >> > spin_lock(&ht->lock);
> >> > - if (tbl->rehash < tbl->size)
> >> > + if (tbl->rcu.func == NULL)
> >> > list_add(&iter->walker.list, &tbl->walkers);
> >> > else
> >> > iter->walker.tbl = NULL;
> >>
> >> This appears to be relying on implementation details within RCU.
> >> Paul, are you OK with rhashtable doing this trick?
> >
> > The notion of accessing objects that are already on RCU's callback lists
> > makes me -very- nervous because this sort of thing is not easy to
> > get right. After all, if you are accessing something that is already
> > on one of RCU's callback lists, RCU might invoke the callback it at any
> > time (thus freeing it in this case), and because it is already on RCU's
> > callback lists, rcu_read_lock() is going to be of no help whatsoever.
>
> I don't follow that last line. If some other thread has already called
> rcu_read_lock() when call_rcu() is called, then that other threads
> rcu_read_lock() will certainly help to ensure that the object doesn't
> get freed. This code assumes that it also ensures that rcu.func will
> not be changed before the other thread calls rcu_read_unlock() and
> allows the grace period to end.
> (There is nothing explicitly about rcu lists here, just rcu.func).
>
> >
> > In addition, RCU does no ordering on its store to ->func, but the ht->lock
> > compensates in this case. But suppose rhashtable_walk_stop() sees the
> > pointer as non-NULL. What prevents RCU from freeing the bucket table out
> > from under rhashtable_walk_stop()? In v4.17, bucket_table_free_rcu()
> > just does some calls to various deallocators, which does not provide
> > the necessary synchronization.
> >
> > Does the rhashtable_iter structure use some trick to make this safe?
> > Or has synchronization been added to bucket_table_free_rcu()? Or is
> > some other trick in use?
> >
> > Thanx, Paul
>
> When rhashtable_rehash_table() has copied all objects out of a
> bucket_table, it must then disconnect any paused walkers and free the
> table. (a 'paused' walker has called rhashtable_walk_stop() and dropped
> the rcu read lock).
> It sets walk->tbl=NULL (thus implicitly removing from the list) and
> calls call_rcu(...,bucket_table_free_rcu) under a spinlock.
>
> When rhashtable_walk_stop() is called, it needs to know whether it is
> safe to attach the walker to the bucket_table().
> It takes the same spin lock as above while still holding the
> rcu_read_lock that it took some time ago.
> If it gets the spinlock before rhashtable_rehash_table() gets it, then
> rcu.func will be NULL (tables are allocated with kzalloc) and the walker
> is attached to the table. If it gets the spinlock after
> rhashtable_rehash_table() gets it, then rcu.func will not be NULL and
> the walker will not be attached to the table.
>
> The only interesting question is whether RCU might ever set rcu.func to
> NULL (or change it at all) *after* call_rcu() has been called, and
> *before* the current grace period ends.
> If you don't want to guarantee that it doesn't, I can add an extra flag
> field to the table to say "this table must not be attached walkers", but
> I currently think that should be unnecessary.
One issue is that the ->func pointer can legitimately be NULL while on
RCU's callback lists. This happens when someone invokes kfree_rcu()
with the rcu_head structure at the beginning of the enclosing structure.
I could add an offset to avoid this, or perhaps the kmalloc() folks
could be persuaded Rao Shoaib's patch moving kfree_rcu() handling to
the slab allocators, so that RCU only ever sees function pointers in
the ->func field.
Either way, this should be hidden behind an API to allow adjustments
to be made if needed. Maybe something like is_after_call_rcu()?
This would (for example) allow debug-object checks to be used to catch
check-after-free bugs.
Would something of that sort work for you?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 2/5] rhashtable: don't hold lock on first table throughout insertion.
From: NeilBrown @ 2018-07-22 23:13 UTC (permalink / raw)
To: paulmck; +Cc: Herbert Xu, Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180722215446.GH12945@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
On Sun, Jul 22 2018, Paul E. McKenney wrote:
>
> One issue is that the ->func pointer can legitimately be NULL while on
> RCU's callback lists. This happens when someone invokes kfree_rcu()
> with the rcu_head structure at the beginning of the enclosing structure.
> I could add an offset to avoid this, or perhaps the kmalloc() folks
> could be persuaded Rao Shoaib's patch moving kfree_rcu() handling to
> the slab allocators, so that RCU only ever sees function pointers in
> the ->func field.
>
> Either way, this should be hidden behind an API to allow adjustments
> to be made if needed. Maybe something like is_after_call_rcu()?
> This would (for example) allow debug-object checks to be used to catch
> check-after-free bugs.
>
> Would something of that sort work for you?
Yes, if you could provide an is_after_call_rcu() API, that would
perfectly suit my use-case.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH net] ip: in cmsg IP(V6)_ORIGDSTADDR do not read beyond headlen
From: Willem de Bruijn @ 2018-07-23 0:43 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Syzbot reported a read beyond the end of the skb head when returning
IPV6_ORIGDSTADDR:
BUG: KMSAN: kernel-infoleak in put_cmsg+0x5ef/0x860 net/core/scm.c:242
CPU: 0 PID: 4501 Comm: syz-executor128 Not tainted 4.17.0+ #9
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x185/0x1d0 lib/dump_stack.c:113
kmsan_report+0x188/0x2a0 mm/kmsan/kmsan.c:1125
kmsan_internal_check_memory+0x138/0x1f0 mm/kmsan/kmsan.c:1219
kmsan_copy_to_user+0x7a/0x160 mm/kmsan/kmsan.c:1261
copy_to_user include/linux/uaccess.h:184 [inline]
put_cmsg+0x5ef/0x860 net/core/scm.c:242
ip6_datagram_recv_specific_ctl+0x1cf3/0x1eb0 net/ipv6/datagram.c:719
ip6_datagram_recv_ctl+0x41c/0x450 net/ipv6/datagram.c:733
rawv6_recvmsg+0x10fb/0x1460 net/ipv6/raw.c:521
[..]
This logic and its ipv4 counterpart read the destination port from
the packet at skb_transport_offset(skb) + 4.
With MSG_MORE and a local SOCK_RAW sender, syzbot was able to cook a
packet that stores headers exactly up to skb_transport_offset(skb) in
the head and the remainder in a frag.
Avoid reading beyond skb->tail by testing skb_headlen(skb) instead of
skb->len when trying to access this field.
Link: http://lkml.kernel.org/r/CAF=yD-LEJwZj5a1-bAAj2Oy_hKmGygV6rsJ_WOrAYnv-fnayiQ@mail.gmail.com
Reported-by: syzbot+9adb4b567003cac781f0@syzkaller.appspotmail.com
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/ipv4/ip_sockglue.c | 2 +-
net/ipv6/datagram.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 64c76dcf7386..5f228d641d9b 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -152,7 +152,7 @@ static void ip_cmsg_recv_dstaddr(struct msghdr *msg, struct sk_buff *skb)
const struct iphdr *iph = ip_hdr(skb);
__be16 *ports = (__be16 *)skb_transport_header(skb);
- if (skb_transport_offset(skb) + 4 > (int)skb->len)
+ if (skb_transport_offset(skb) + 4 > (int)skb_headlen(skb))
return;
/* All current transport protocols have the port numbers in the
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 2ee08b6a86a4..fd43810aacd2 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -702,7 +702,7 @@ void ip6_datagram_recv_specific_ctl(struct sock *sk, struct msghdr *msg,
struct sockaddr_in6 sin6;
__be16 *ports = (__be16 *) skb_transport_header(skb);
- if (skb_transport_offset(skb) + 4 <= (int)skb->len) {
+ if (skb_transport_offset(skb) + 4 <= (int)skb_headlen(skb)) {
/* All current transport protocols have the port numbers in the
* first four bytes of the transport header and this function is
* written with this assumption in mind.
--
2.18.0.233.g985f88cf7e-goog
^ permalink raw reply related
* [PATCH net-next] rhashtable: detect when object movement between tables might have invalidated a lookup
From: NeilBrown @ 2018-07-23 1:56 UTC (permalink / raw)
To: David Miller; +Cc: herbert, tgraf, netdev, linux-kernel, eric.dumazet
In-Reply-To: <20180719.051440.931407144963903326.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 3822 bytes --]
Some users of rhashtables might need to move an object from one table
to another - this appears to be the reason for the incomplete usage
of NULLS markers.
To support these, we store a unique NULLS_MARKER at the end of
each chain, and when a search fails to find a match, we check
if the NULLS marker found was the expected one. If not,
the search is repeated.
The unique NULLS_MARKER is derived from the address of the
head of the chain. As this cannot be derived at load-time the
static rhnull in rht_bucket_nexted() need to be initialised
at run time.
Any caller of a lookup function must be prepared for the possibility
that the object returned is in a different table - it might have been
there for some time.
Note that this does NOT provide support for other uses for
NULLS_MARKERs such as allocating with SLAB_TYPESAFE_BY_RCU or changing
the key of an object and re-inserting it in the table.
These could only be done safely if new objects were inserted
at the *start* of a hash chain, and that is not currently the case.
Signed-off-by: NeilBrown <neilb@suse.com>
---
This is a simplified version of a previous patch.
It provides NULLS_MARKER support only for the specific use case
which is currently thought be valuable to in-tree users
of rhashtables.
Thanks,
NeilBrown
include/linux/rhashtable.h | 25 +++++++++++++++++--------
lib/rhashtable.c | 3 +--
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index eb7111039247..8cc240f14834 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -75,8 +75,10 @@ struct bucket_table {
struct rhash_head __rcu *buckets[] ____cacheline_aligned_in_smp;
};
+#define RHT_NULLS_MARKER(ptr) \
+ ((void *)NULLS_MARKER(((unsigned long) (ptr)) >> 1))
#define INIT_RHT_NULLS_HEAD(ptr) \
- ((ptr) = (typeof(ptr)) NULLS_MARKER(0))
+ ((ptr) = RHT_NULLS_MARKER(&(ptr)))
static inline bool rht_is_a_nulls(const struct rhash_head *ptr)
{
@@ -471,6 +473,7 @@ static inline struct rhash_head *__rhashtable_lookup(
.ht = ht,
.key = key,
};
+ struct rhash_head __rcu * const *head;
struct bucket_table *tbl;
struct rhash_head *he;
unsigned int hash;
@@ -478,13 +481,19 @@ static inline struct rhash_head *__rhashtable_lookup(
tbl = rht_dereference_rcu(ht->tbl, ht);
restart:
hash = rht_key_hashfn(ht, tbl, key, params);
- rht_for_each_rcu(he, tbl, hash) {
- if (params.obj_cmpfn ?
- params.obj_cmpfn(&arg, rht_obj(ht, he)) :
- rhashtable_compare(&arg, rht_obj(ht, he)))
- continue;
- return he;
- }
+ head = rht_bucket(tbl, hash);
+ do {
+ rht_for_each_rcu_continue(he, *head, tbl, hash) {
+ if (params.obj_cmpfn ?
+ params.obj_cmpfn(&arg, rht_obj(ht, he)) :
+ rhashtable_compare(&arg, rht_obj(ht, he)))
+ continue;
+ return he;
+ }
+ /* An object might have been moved to a different hash chain,
+ * while we walk along it - better check and retry.
+ */
+ } while (he != RHT_NULLS_MARKER(head));
/* Ensure we see any new tables. */
smp_rmb();
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index ae4223e0f5bc..ac48f026a8c3 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -1175,8 +1175,7 @@ struct rhash_head __rcu **rht_bucket_nested(const struct bucket_table *tbl,
unsigned int hash)
{
const unsigned int shift = PAGE_SHIFT - ilog2(sizeof(void *));
- static struct rhash_head __rcu *rhnull =
- (struct rhash_head __rcu *)NULLS_MARKER(0);
+ static struct rhash_head __rcu *rhnull = RHT_NULLS_MARKER(&rhnull);
unsigned int index = hash & ((1 << tbl->nest) - 1);
unsigned int size = tbl->size >> tbl->nest;
unsigned int subhash = hash;
--
2.14.0.rc0.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related
* Re: [PATCH] netlink: fix memory leak of dump
From: shaochun chen @ 2018-07-23 2:05 UTC (permalink / raw)
To: Florian Westphal
Cc: David Miller, pablo, kadlec, johannes.berg, jason, ktkhai,
lucien.xin, xiyou.wangcong, dsahern, netfilter-devel, tom, netdev,
linux-kernel
In-Reply-To: <20180722180910.wcwhantwpm2nfxet@breakpoint.cc>
[-- Attachment #1: Type: text/plain, Size: 3400 bytes --]
allocate memory in cb->start(), which means passing 'static' variable
through control->data,
then allocate memory in cb->start() according to cb->data (cb->data is
equal to control->data now),
and set the memory back to cb->data which will be used in cb->dump().
It's a bit complicated， please see nf_tables_getset.
2018-07-23 2:09 GMT+08:00 Florian Westphal <fw@strlen.de>:
> David Miller <davem@davemloft.net> wrote:
> > From: Florian Westphal <fw@strlen.de>
> > Date: Sun, 22 Jul 2018 18:39:25 +0200
> >
> > > 3. change meaning of ->done() so its always called once ->start()
> > > was invoked (and returned 0), this requires audit of all
> > > places that provide .done to make sure they won't trip.
> > >
> > > 3) seems to be what Tom intended when he added .start, so probably
> > > best to investigate that first.
> >
> > Hmmm...
> >
> > Any time ->start() succeeds, we set cb_running to true.
>
> Right.
>
> > From that point forward, ->done() will be called at some point at all
> > of the locations that check if cb_running is true and set it to false.
>
> Also right, thanks for pointing this out, I missed fact that netlink
> core restarts a dump after this.
>
> So 3) is already true which means we should try to see if we can move
> all dump-related extra magic into ->start().
>
> Shaochun, can you see if this is possible?
>
> Something along these lines (totally untested), which makes this
> a netfilter fix:
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const
> nla[])
> return filter;
> }
>
> +static int nf_tables_dump_obj_start(struct netlink_callback *cb)
> +{
> + const struct nlattr * const *nla = cb->data;
> + struct nft_obj_filter *filter = NULL;
> +
> + if (nla[NFTA_OBJ_TABLE] ||
> + nla[NFTA_OBJ_TYPE]) {
> + filter = nft_obj_filter_alloc(nla);
> + if (IS_ERR(filter))
> + return -ENOMEM;
> + }
> +
> + cb->data = filter;
> + return 0;
> +}
> +
> /* called with rcu_read_lock held */
> static int nf_tables_getobj(struct net *net, struct sock *nlsk,
> struct sk_buff *skb, const struct nlmsghdr
> *nlh,
> @@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net,
> struct sock *nlsk,
>
> if (nlh->nlmsg_flags & NLM_F_DUMP) {
> struct netlink_dump_control c = {
> + .start = nf_tables_dump_obj_start,
> .dump = nf_tables_dump_obj,
> .done = nf_tables_dump_obj_done,
> .module = THIS_MODULE,
> + .data = (void *)nla,
> };
>
> - if (nla[NFTA_OBJ_TABLE] ||
> - nla[NFTA_OBJ_TYPE]) {
> - struct nft_obj_filter *filter;
> -
> - filter = nft_obj_filter_alloc(nla);
> - if (IS_ERR(filter))
> - return -ENOMEM;
> -
> - c.data = filter;
> - }
> return nft_netlink_dump_start_rcu(nlsk, skb, nlh, &c);
> }
>
>
[-- Attachment #2: Type: text/html, Size: 4575 bytes --]
^ permalink raw reply
* Re: [PATCH] net/9p/trans_fd.c: fix double list_del() and race in access
From: jiangyiwen @ 2018-07-23 2:24 UTC (permalink / raw)
To: Tomas Bortoli, ericvh, rminnich, lucho
Cc: davem, v9fs-developer, netdev, linux-kernel, syzkaller
In-Reply-To: <20180720132801.22749-1-tomasbortoli@gmail.com>
On 2018/7/20 21:28, Tomas Bortoli wrote:
> This patch uses list_del_init() instead of list_del() to eliminate "req_list". This to prevent double list_del()'s calls to the same list from provoking a GPF. Furthermore, this patch fixes an access to "req_list" that was made without getting the relative lock.
>
I suggest you can apply a 72-character line limit to your
commit messages. Others looks good to me.
Thanks,
Yiwen.
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+735d926e9d1317c3310c@syzkaller.appspotmail.com
> ---
>
> net/9p/trans_fd.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index a64b01c56e30..131bb1f059e6 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -223,7 +223,9 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
>
> list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
> p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
> - list_del(&req->req_list);
> + spin_lock_irqsave(&m->client->lock, flags);
> + list_del_init(&req->req_list);
> + spin_unlock_irqrestore(&m->client->lock, flags);
> if (!req->t_err)
> req->t_err = err;
> p9_client_cb(m->client, req, REQ_STATUS_ERROR);
> @@ -369,7 +371,7 @@ static void p9_read_work(struct work_struct *work)
> spin_lock(&m->client->lock);
> if (m->req->status != REQ_STATUS_ERROR)
> status = REQ_STATUS_RCVD;
> - list_del(&m->req->req_list);
> + list_del_init(&m->req->req_list);
> spin_unlock(&m->client->lock);
> p9_client_cb(m->client, m->req, status);
> m->rc.sdata = NULL;
> @@ -684,7 +686,7 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req)
> spin_lock(&client->lock);
>
> if (req->status == REQ_STATUS_UNSENT) {
> - list_del(&req->req_list);
> + list_del_init(&req->req_list);
> req->status = REQ_STATUS_FLSHD;
> ret = 0;
> }
> @@ -701,7 +703,7 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
> * remove it from the list.
> */
> spin_lock(&client->lock);
> - list_del(&req->req_list);
> + list_del_init(&req->req_list);
> spin_unlock(&client->lock);
>
> return 0;
>
^ permalink raw reply
* Re: [PATCH] [V9fs-developer] [PATCH] /net/9p/trans_fd.c: fix race-condition by flushing workqueue before the kfree()
From: Dominique Martinet @ 2018-07-23 2:39 UTC (permalink / raw)
To: Tomas Bortoli
Cc: ericvh, rminnich, lucho, jiangyiwen, davem, v9fs-developer,
netdev, linux-kernel, syzkaller
In-Reply-To: <20180720092730.27104-1-tomasbortoli@gmail.com>
Tomas Bortoli wrote on Fri, Jul 20, 2018:
> The patch adds the flush in p9_mux_poll_stop() as it the function used by
> p9_conn_destroy(), in turn called by p9_fd_close() to stop the async
> polling associated with the data regarding the connection.
>
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+39749ed7d9ef6dfb23f6@syzkaller.appspotmail.com
Looks good to me, I'm taking this patch.
If I had to say something, try to aim for slightly shorter subject lines
if possible :)
> ---
> As shown by Syzbot, it is possible to provoke a race between p9_fd_close()
> and p9_poll_workfn() that is called to take care of the async read/write work
> to do. To make sure p9_fd_close() frees "trans" when it is not used anymore,
> it has to explicitly flush p9_poll_work before the kfree().
>
> net/9p/trans_fd.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index bf459ee0feab..a64b01c56e30 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -185,6 +185,8 @@ static void p9_mux_poll_stop(struct p9_conn *m)
> spin_lock_irqsave(&p9_poll_lock, flags);
> list_del_init(&m->poll_pending_link);
> spin_unlock_irqrestore(&p9_poll_lock, flags);
> +
> + flush_work(&p9_poll_work);
> }
>
> /**
--
Dominique Martinet
^ 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