Message ID | 20250305163732.2766420-9-sdf@fomichev.me (mailing list archive) |
---|---|
State | Accepted |
Commit | ad7c7b2172c388818a111455643491d75f535e90 |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: Hold netdev instance lock during ndo operations | expand |
On Wed, 2025-03-05 at 08:37 -0800, Stanislav Fomichev wrote: > diff --git a/net/core/dev_api.c b/net/core/dev_api.c > index 059413d9ef9d..7bd667b34b80 100644 > --- a/net/core/dev_api.c > +++ b/net/core/dev_api.c > + > +/** > + * dev_disable_lro() - disable Large Receive Offload on a device > + * @dev: device > + * > + * Disable Large Receive Offload (LRO) on a net device. Must be > + * called under RTNL. This is needed if received packets may be > + * forwarded to another interface. > + */ > +void dev_disable_lro(struct net_device *dev) > +{ > + netdev_lock_ops(dev); > + netif_disable_lro(dev); > + netdev_unlock_ops(dev); > +} It seems this part plus the following part from patch 6 of this series result in a recursive deadlock when inet forwarding is not enabled: > @@ -3013,6 +3021,8 @@ static int do_setlink(const struct sk_buff > *skb, struct net_device *dev, > char ifname[IFNAMSIZ]; > int err; > > + netdev_lock_ops(dev); > + > err = validate_linkmsg(dev, tb, extack); > if (err < 0) > goto errout; > Call Trace: dump_stack_lvl+0x62/0x90 print_deadlock_bug+0x274/0x3b0 __lock_acquire+0x1229/0x2470 lock_acquire+0xb7/0x2b0 __mutex_lock+0xa6/0xd20 dev_disable_lro+0x20/0x80 inetdev_init+0x12f/0x1f0 inetdev_event+0x48b/0x870 notifier_call_chain+0x38/0xf0 netif_change_net_namespace+0x72e/0x9f0 do_setlink.isra.0+0xd5/0x1220 rtnl_newlink+0x7ea/0xb50 rtnetlink_rcv_msg+0x459/0x5e0 netlink_rcv_skb+0x54/0x100 netlink_unicast+0x193/0x270 netlink_sendmsg+0x204/0x450 inetdev_init conditionally disables LRO if forwarding is on: if (IPV4_DEVCONF(in_dev->cnf, FORWARDING)) dev_disable_lro(dev); What to do in this case (besides the silly workaround to disable forwarding)? Cosmin.
On Mon, 2025-03-24 at 15:34 +0000, Cosmin Ratiu wrote: > It seems this part plus the following part from patch 6 of this > series > result in a recursive deadlock when inet forwarding is not enabled: Correction: This should read "when inet forwarding is enabled".
On 03/24, Cosmin Ratiu wrote: > On Wed, 2025-03-05 at 08:37 -0800, Stanislav Fomichev wrote: > > diff --git a/net/core/dev_api.c b/net/core/dev_api.c > > index 059413d9ef9d..7bd667b34b80 100644 > > --- a/net/core/dev_api.c > > +++ b/net/core/dev_api.c > > + > > +/** > > + * dev_disable_lro() - disable Large Receive Offload on a device > > + * @dev: device > > + * > > + * Disable Large Receive Offload (LRO) on a net device. Must be > > + * called under RTNL. This is needed if received packets may be > > + * forwarded to another interface. > > + */ > > +void dev_disable_lro(struct net_device *dev) > > +{ > > + netdev_lock_ops(dev); > > + netif_disable_lro(dev); > > + netdev_unlock_ops(dev); > > +} > > It seems this part plus the following part from patch 6 of this series > result in a recursive deadlock when inet forwarding is not enabled: > > > @@ -3013,6 +3021,8 @@ static int do_setlink(const struct sk_buff > > *skb, struct net_device *dev, > > char ifname[IFNAMSIZ]; > > int err; > > > > + netdev_lock_ops(dev); > > + > > err = validate_linkmsg(dev, tb, extack); > > if (err < 0) > > goto errout; > > > > Call Trace: > dump_stack_lvl+0x62/0x90 > print_deadlock_bug+0x274/0x3b0 > __lock_acquire+0x1229/0x2470 > lock_acquire+0xb7/0x2b0 > __mutex_lock+0xa6/0xd20 > dev_disable_lro+0x20/0x80 > inetdev_init+0x12f/0x1f0 > inetdev_event+0x48b/0x870 > notifier_call_chain+0x38/0xf0 > netif_change_net_namespace+0x72e/0x9f0 > do_setlink.isra.0+0xd5/0x1220 > rtnl_newlink+0x7ea/0xb50 > rtnetlink_rcv_msg+0x459/0x5e0 > netlink_rcv_skb+0x54/0x100 > netlink_unicast+0x193/0x270 > netlink_sendmsg+0x204/0x450 > > inetdev_init conditionally disables LRO if forwarding is on: > if (IPV4_DEVCONF(in_dev->cnf, FORWARDING)) > dev_disable_lro(dev); > > What to do in this case (besides the silly workaround to disable > forwarding)? I think something like the patch below should fix it? inetdev_init is called for blackhole (sw device, we don't care about ops lock) and from REGISTER/UNREGISTER notifiers. We hold the lock during REGISTER, and will soon hold the lock during UNREGISTER: https://lore.kernel.org/netdev/20250312223507.805719-9-kuba@kernel.org/ (might also need to EXPORT_SYM netif_disable_lro) diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index 754f60fb6e25..77e5705ac799 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -281,7 +281,7 @@ static struct in_device *inetdev_init(struct net_device *dev) if (!in_dev->arp_parms) goto out_kfree; if (IPV4_DEVCONF(in_dev->cnf, FORWARDING)) - dev_disable_lro(dev); + netif_disable_lro(dev); /* Reference in_dev->dev */ netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL); /* Account for reference dev->ip_ptr (below) */
On Mon, 2025-03-24 at 09:06 -0700, Stanislav Fomichev wrote: > On 03/24, Cosmin Ratiu wrote: > > Call Trace: > > dump_stack_lvl+0x62/0x90 > > print_deadlock_bug+0x274/0x3b0 > > __lock_acquire+0x1229/0x2470 > > lock_acquire+0xb7/0x2b0 > > __mutex_lock+0xa6/0xd20 > > dev_disable_lro+0x20/0x80 > > inetdev_init+0x12f/0x1f0 > > inetdev_event+0x48b/0x870 > > notifier_call_chain+0x38/0xf0 > > netif_change_net_namespace+0x72e/0x9f0 > > do_setlink.isra.0+0xd5/0x1220 > > rtnl_newlink+0x7ea/0xb50 > > rtnetlink_rcv_msg+0x459/0x5e0 > > netlink_rcv_skb+0x54/0x100 > > netlink_unicast+0x193/0x270 > > netlink_sendmsg+0x204/0x450 > > I think something like the patch below should fix it? inetdev_init is > called for blackhole (sw device, we don't care about ops lock) and > from > REGISTER/UNREGISTER notifiers. We hold the lock during REGISTER, > and will soon hold the lock during UNREGISTER: > https://lore.kernel.org/netdev/20250312223507.805719-9-kuba@kernel.org/ > > (might also need to EXPORT_SYM netif_disable_lro) > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index 754f60fb6e25..77e5705ac799 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -281,7 +281,7 @@ static struct in_device *inetdev_init(struct > net_device *dev) > if (!in_dev->arp_parms) > goto out_kfree; > if (IPV4_DEVCONF(in_dev->cnf, FORWARDING)) > - dev_disable_lro(dev); > + netif_disable_lro(dev); > /* Reference in_dev->dev */ > netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL); > /* Account for reference dev->ip_ptr (below) */ Unfortunately, this seems to result, on another code path, in: WARNING: CPU: 10 PID: 1479 at ./include/net/netdev_lock.h:54 __netdev_update_features+0x65f/0xca0 __warn+0x81/0x180 __netdev_update_features+0x65f/0xca0 report_bug+0x156/0x180 handle_bug+0x4f/0x90 exc_invalid_op+0x13/0x60 asm_exc_invalid_op+0x16/0x20 __netdev_update_features+0x65f/0xca0 netif_disable_lro+0x30/0x1d0 inetdev_init+0x12f/0x1f0 inetdev_event+0x48b/0x870 notifier_call_chain+0x38/0xf0 register_netdevice+0x741/0x8b0 register_netdev+0x1f/0x40 mlx5e_probe+0x4e3/0x8e0 [mlx5_core] auxiliary_bus_probe+0x3f/0x90 really_probe+0xc3/0x3a0 __driver_probe_device+0x80/0x150 driver_probe_device+0x1f/0x90 __device_attach_driver+0x7d/0x100 bus_for_each_drv+0x80/0xd0 __device_attach+0xb4/0x1c0 bus_probe_device+0x91/0xa0 device_add+0x657/0x870 I see register_netdevice briefly acquires the netdev lock in two separate blocks and has a __netdev_update_features call in one of the blocks, but the lock is not held for call_netdevice_notifiers(NETDEV_REGISTER, dev). Cosmin.
On 03/24, Cosmin Ratiu wrote: > On Mon, 2025-03-24 at 09:06 -0700, Stanislav Fomichev wrote: > > On 03/24, Cosmin Ratiu wrote: > > > Call Trace: > > > dump_stack_lvl+0x62/0x90 > > > print_deadlock_bug+0x274/0x3b0 > > > __lock_acquire+0x1229/0x2470 > > > lock_acquire+0xb7/0x2b0 > > > __mutex_lock+0xa6/0xd20 > > > dev_disable_lro+0x20/0x80 > > > inetdev_init+0x12f/0x1f0 > > > inetdev_event+0x48b/0x870 > > > notifier_call_chain+0x38/0xf0 > > > netif_change_net_namespace+0x72e/0x9f0 > > > do_setlink.isra.0+0xd5/0x1220 > > > rtnl_newlink+0x7ea/0xb50 > > > rtnetlink_rcv_msg+0x459/0x5e0 > > > netlink_rcv_skb+0x54/0x100 > > > netlink_unicast+0x193/0x270 > > > netlink_sendmsg+0x204/0x450 > > > > I think something like the patch below should fix it? inetdev_init is > > called for blackhole (sw device, we don't care about ops lock) and > > from > > REGISTER/UNREGISTER notifiers. We hold the lock during REGISTER, > > and will soon hold the lock during UNREGISTER: > > https://lore.kernel.org/netdev/20250312223507.805719-9-kuba@kernel.org/ > > > > (might also need to EXPORT_SYM netif_disable_lro) > > > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > > index 754f60fb6e25..77e5705ac799 100644 > > --- a/net/ipv4/devinet.c > > +++ b/net/ipv4/devinet.c > > @@ -281,7 +281,7 @@ static struct in_device *inetdev_init(struct > > net_device *dev) > > if (!in_dev->arp_parms) > > goto out_kfree; > > if (IPV4_DEVCONF(in_dev->cnf, FORWARDING)) > > - dev_disable_lro(dev); > > + netif_disable_lro(dev); > > /* Reference in_dev->dev */ > > netdev_hold(dev, &in_dev->dev_tracker, GFP_KERNEL); > > /* Account for reference dev->ip_ptr (below) */ > > Unfortunately, this seems to result, on another code path, in: > WARNING: CPU: 10 PID: 1479 at ./include/net/netdev_lock.h:54 > __netdev_update_features+0x65f/0xca0 > __warn+0x81/0x180 > __netdev_update_features+0x65f/0xca0 > report_bug+0x156/0x180 > handle_bug+0x4f/0x90 > exc_invalid_op+0x13/0x60 > asm_exc_invalid_op+0x16/0x20 > __netdev_update_features+0x65f/0xca0 > netif_disable_lro+0x30/0x1d0 > inetdev_init+0x12f/0x1f0 > inetdev_event+0x48b/0x870 > notifier_call_chain+0x38/0xf0 > register_netdevice+0x741/0x8b0 > register_netdev+0x1f/0x40 > mlx5e_probe+0x4e3/0x8e0 [mlx5_core] > auxiliary_bus_probe+0x3f/0x90 > really_probe+0xc3/0x3a0 > __driver_probe_device+0x80/0x150 > driver_probe_device+0x1f/0x90 > __device_attach_driver+0x7d/0x100 > bus_for_each_drv+0x80/0xd0 > __device_attach+0xb4/0x1c0 > bus_probe_device+0x91/0xa0 > device_add+0x657/0x870 > > I see register_netdevice briefly acquires the netdev lock in two > separate blocks and has a __netdev_update_features call in one of the > blocks, but the lock is not held for > call_netdevice_notifiers(NETDEV_REGISTER, dev). Ok, so we might need to also try to run NETDEV_REGISTER hooks consistently under the instance lock. This might bring more surprises, but I don't see any other easy option. Will test it out locally... diff --git a/net/core/dev.c b/net/core/dev.c index f29c1368c304..d672d521b92a 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1815,7 +1815,9 @@ static int call_netdevice_register_notifiers(struct notifier_block *nb, { int err; + netdev_lock_ops(dev); err = call_netdevice_notifier(nb, NETDEV_REGISTER, dev); + netdev_unlock_ops(dev); err = notifier_to_errno(err); if (err) return err; @@ -11014,7 +11016,9 @@ int register_netdevice(struct net_device *dev) memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len); /* Notify protocols, that a new device appeared. */ + netdev_lock_ops(dev); ret = call_netdevice_notifiers(NETDEV_REGISTER, dev); + netdev_unlock_ops(dev); ret = notifier_to_errno(ret); if (ret) { /* Expect explicit free_netdev() on failure */ @@ -12036,6 +12040,7 @@ int netif_change_net_namespace(struct net_device *dev, struct net *net, int err, new_nsid; ASSERT_RTNL(); + netdev_ops_assert_locked(dev); /* Don't allow namespace local devices to be moved. */ err = -EINVAL;
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index cd4afeb9ad42..cf0b02720dd8 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2644,10 +2644,13 @@ static int __bond_release_one(struct net_device *bond_dev, dev_set_mac_address(slave_dev, (struct sockaddr *)&ss, NULL); } - if (unregister) + if (unregister) { + netdev_lock_ops(slave_dev); __dev_set_mtu(slave_dev, slave->original_mtu); - else + netdev_unlock_ops(slave_dev); + } else { dev_set_mtu(slave_dev, slave->original_mtu); + } if (!netif_is_bond_master(slave_dev)) slave_dev->priv_flags &= ~IFF_BONDING; diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 8d243c0ec39d..c61b12809588 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3382,6 +3382,7 @@ void dev_close(struct net_device *dev); void dev_close_many(struct list_head *head, bool unlink); int dev_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data); +void netif_disable_lro(struct net_device *dev); void dev_disable_lro(struct net_device *dev); int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *newskb); u16 dev_pick_tx_zero(struct net_device *dev, struct sk_buff *skb, @@ -4257,6 +4258,8 @@ int netif_set_mtu(struct net_device *dev, int new_mtu); int dev_set_mtu(struct net_device *, int); int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr, struct netlink_ext_ack *extack); +int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa, + struct netlink_ext_ack *extack); int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, struct netlink_ext_ack *extack); int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, @@ -5016,6 +5019,7 @@ static inline void __dev_mc_unsync(struct net_device *dev, /* Functions used for secondary unicast and multicast support */ void dev_set_rx_mode(struct net_device *dev); int dev_set_promiscuity(struct net_device *dev, int inc); +int netif_set_allmulti(struct net_device *dev, int inc, bool notify); int dev_set_allmulti(struct net_device *dev, int inc); void netdev_state_change(struct net_device *dev); void __netdev_notify_peers(struct net_device *dev); diff --git a/net/core/dev.c b/net/core/dev.c index 97a4fc0889d3..121c0449f87f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1757,15 +1757,7 @@ int dev_setup_tc(struct net_device *dev, enum tc_setup_type type, } EXPORT_SYMBOL(dev_setup_tc); -/** - * dev_disable_lro - disable Large Receive Offload on a device - * @dev: device - * - * Disable Large Receive Offload (LRO) on a net device. Must be - * called under RTNL. This is needed if received packets may be - * forwarded to another interface. - */ -void dev_disable_lro(struct net_device *dev) +void netif_disable_lro(struct net_device *dev) { struct net_device *lower_dev; struct list_head *iter; @@ -1776,10 +1768,12 @@ void dev_disable_lro(struct net_device *dev) if (unlikely(dev->features & NETIF_F_LRO)) netdev_WARN(dev, "failed to disable LRO!\n"); - netdev_for_each_lower_dev(dev, lower_dev, iter) - dev_disable_lro(lower_dev); + netdev_for_each_lower_dev(dev, lower_dev, iter) { + netdev_lock_ops(lower_dev); + netif_disable_lro(lower_dev); + netdev_unlock_ops(lower_dev); + } } -EXPORT_SYMBOL(dev_disable_lro); /** * dev_disable_gro_hw - disable HW Generic Receive Offload on a device @@ -6038,7 +6032,7 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp) static_branch_dec(&generic_xdp_needed_key); } else if (new && !old) { static_branch_inc(&generic_xdp_needed_key); - dev_disable_lro(dev); + netif_disable_lro(dev); dev_disable_gro_hw(dev); } break; @@ -9210,7 +9204,7 @@ int dev_set_promiscuity(struct net_device *dev, int inc) } EXPORT_SYMBOL(dev_set_promiscuity); -static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify) +int netif_set_allmulti(struct net_device *dev, int inc, bool notify) { unsigned int old_flags = dev->flags, old_gflags = dev->gflags; unsigned int allmulti, flags; @@ -9245,25 +9239,6 @@ static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify) return 0; } -/** - * dev_set_allmulti - update allmulti count on a device - * @dev: device - * @inc: modifier - * - * Add or remove reception of all multicast frames to a device. While the - * count in the device remains above zero the interface remains listening - * to all interfaces. Once it hits zero the device reverts back to normal - * filtering operation. A negative @inc value is used to drop the counter - * when releasing a resource needing all multicasts. - * Return 0 if successful or a negative errno code on error. - */ - -int dev_set_allmulti(struct net_device *dev, int inc) -{ - return __dev_set_allmulti(dev, inc, true); -} -EXPORT_SYMBOL(dev_set_allmulti); - /* * Upload unicast and multicast address lists to device and * configure RX filtering. When the device doesn't support unicast @@ -9396,7 +9371,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags, int inc = (flags & IFF_ALLMULTI) ? 1 : -1; dev->gflags ^= IFF_ALLMULTI; - __dev_set_allmulti(dev, inc, false); + netif_set_allmulti(dev, inc, false); } return ret; @@ -9588,16 +9563,8 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr, } EXPORT_SYMBOL(dev_pre_changeaddr_notify); -/** - * dev_set_mac_address - Change Media Access Control Address - * @dev: device - * @sa: new address - * @extack: netlink extended ack - * - * Change the hardware (MAC) address of the device - */ -int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, - struct netlink_ext_ack *extack) +int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa, + struct netlink_ext_ack *extack) { const struct net_device_ops *ops = dev->netdev_ops; int err; @@ -9621,7 +9588,6 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, add_device_randomness(dev->dev_addr, dev->addr_len); return 0; } -EXPORT_SYMBOL(dev_set_mac_address); DECLARE_RWSEM(dev_addr_sem); @@ -9631,7 +9597,7 @@ int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, int ret; down_write(&dev_addr_sem); - ret = dev_set_mac_address(dev, sa, extack); + ret = netif_set_mac_address(dev, sa, extack); up_write(&dev_addr_sem); return ret; } diff --git a/net/core/dev_api.c b/net/core/dev_api.c index 059413d9ef9d..7bd667b34b80 100644 --- a/net/core/dev_api.c +++ b/net/core/dev_api.c @@ -252,3 +252,68 @@ int dev_set_mtu(struct net_device *dev, int new_mtu) return ret; } EXPORT_SYMBOL(dev_set_mtu); + +/** + * dev_disable_lro() - disable Large Receive Offload on a device + * @dev: device + * + * Disable Large Receive Offload (LRO) on a net device. Must be + * called under RTNL. This is needed if received packets may be + * forwarded to another interface. + */ +void dev_disable_lro(struct net_device *dev) +{ + netdev_lock_ops(dev); + netif_disable_lro(dev); + netdev_unlock_ops(dev); +} +EXPORT_SYMBOL(dev_disable_lro); + +/** + * dev_set_allmulti() - update allmulti count on a device + * @dev: device + * @inc: modifier + * + * Add or remove reception of all multicast frames to a device. While the + * count in the device remains above zero the interface remains listening + * to all interfaces. Once it hits zero the device reverts back to normal + * filtering operation. A negative @inc value is used to drop the counter + * when releasing a resource needing all multicasts. + * + * Return: 0 on success, -errno on failure. + */ + +int dev_set_allmulti(struct net_device *dev, int inc) +{ + int ret; + + netdev_lock_ops(dev); + ret = netif_set_allmulti(dev, inc, true); + netdev_unlock_ops(dev); + + return ret; +} +EXPORT_SYMBOL(dev_set_allmulti); + +/** + * dev_set_mac_address() - change Media Access Control Address + * @dev: device + * @sa: new address + * @extack: netlink extended ack + * + * Change the hardware (MAC) address of the device + * + * Return: 0 on success, -errno on failure. + */ +int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, + struct netlink_ext_ack *extack) +{ + int ret; + + netdev_lock_ops(dev); + ret = netif_set_mac_address(dev, sa, extack); + netdev_unlock_ops(dev); + + return ret; +} +EXPORT_SYMBOL(dev_set_mac_address); diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 8d9dc048a548..27eea34d1b41 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1483,8 +1483,10 @@ static ssize_t tx_maxrate_store(struct kobject *kobj, struct attribute *attr, return err; err = -EOPNOTSUPP; + netdev_lock_ops(dev); if (dev->netdev_ops->ndo_set_tx_maxrate) err = dev->netdev_ops->ndo_set_tx_maxrate(dev, index, rate); + netdev_unlock_ops(dev); if (!err) { queue->tx_maxrate = rate;
Most of them are already covered by the converted dev_xxx APIs. Add the locking wrappers for the remaining ones. Cc: Saeed Mahameed <saeed@kernel.org> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- drivers/net/bonding/bond_main.c | 7 +++- include/linux/netdevice.h | 4 ++ net/core/dev.c | 58 ++++++----------------------- net/core/dev_api.c | 65 +++++++++++++++++++++++++++++++++ net/core/net-sysfs.c | 2 + 5 files changed, 88 insertions(+), 48 deletions(-)