lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 29 Jan 2017 16:06:13 +0100 From: Lino Sanfilippo <LinoSanfilippo@....de> To: Alexander Loktionov <Alexander.Loktionov@...antia.com>, netdev@...r.kernel.org, David VomLehn <vomlehn@...as.net> Cc: "David S . Miller" <davem@...emloft.net>, Simon Edelhaus <Simon.Edelhaus@...antia.com>, Dmitrii Tarakanov <Dmitrii.Tarakanov@...antia.com>, Pavel Belous <Pavel.Belous@...antia.com>, Dmitry Bezrukov <Dmitry.Bezrukov@...antia.com> Subject: Re: [PATCH stable v1 05/13] net: ethernet: aquantia: Support for NIC-specific code On 29.01.2017 06:09, Alexander Loktionov wrote: > + > +static int aq_ndev_start_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct aq_nic_s *aq_nic = netdev_priv(ndev); > + int err = 0; > + > + err = aq_nic_xmit(aq_nic, skb); Initialization of err is superfluous. > + if (err < 0) > + goto err_exit; > + > +err_exit: > + return err; > +} > + > +static int aq_ndev_change_mtu(struct net_device *ndev, int new_mtu) > +{ > + struct aq_nic_s *aq_nic = netdev_priv(ndev); > + int err = 0; > + > + if (new_mtu == ndev->mtu) { This check is not needed. This function wont be called if mtu has not changed. > + > +static int aq_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *pci_id) > +{ > + struct aq_hw_ops *aq_hw_ops = NULL; > + struct aq_pci_func_s *aq_pci_func = NULL; > + int err = 0; > + > + err = pci_enable_device(pdev); > + if (err < 0) > + goto err_exit; > + aq_hw_ops = aq_pci_probe_get_hw_ops_by_id(pdev); > + aq_pci_func = aq_pci_func_alloc(aq_hw_ops, pdev, > + &aq_ndev_ops, &aq_ethtool_ops); > + if (!aq_pci_func) { pci_disable_device() is missing. > + > +static int __init aq_module_init(void) > +{ > + int err = 0; > + > + err = pci_register_driver(&aq_pci_ops); > + if (err < 0) > + goto err_exit; > + > +err_exit: > + return err; > +} > + > +static void __exit aq_module_exit(void) > +{ > + pci_unregister_driver(&aq_pci_ops); > +} > This can be reduced to a single line: module_pci_driver(aq_pci_ops); > + > +static void aq_nic_service_timer_cb(unsigned long param) > +{ > + struct aq_nic_s *self = (struct aq_nic_s *)param; > + struct net_device *ndev = aq_nic_get_ndev(self); > + int err = 0; > + bool is_busy = false; > + unsigned int i = 0U; > + struct aq_hw_link_status_s link_status; > + struct aq_ring_stats_rx_s stats_rx; > + struct aq_ring_stats_tx_s stats_tx; > + > + atomic_inc(&self->header.busy_count); > + is_busy = true; What is "is_busy" needed for? Furthermore busy_count seems to be meant for synchronization with the xmit function. Why do they have to be synchronized? > + > + ndev->stats.rx_packets = stats_rx.packets; > + ndev->stats.rx_bytes = stats_rx.bytes; > + ndev->stats.rx_errors = stats_rx.errors; > + ndev->stats.tx_packets = stats_tx.packets; > + ndev->stats.tx_bytes = stats_tx.bytes; > + ndev->stats.tx_errors = stats_tx.errors; You should consider the use of u64_stats_update_begin() and friends to update and retrieve statistics. Also why do you have to update the stats in a timer? > + > +struct aq_nic_s *aq_nic_alloc_cold(const struct net_device_ops *ndev_ops, > + const struct ethtool_ops *et_ops, > + struct device *dev, > + struct aq_pci_func_s *aq_pci_func, > + unsigned int port, > + const struct aq_hw_ops *aq_hw_ops) > +{ > + struct net_device *ndev = NULL; > + struct aq_nic_s *self = NULL; > + int err = 0; > + > + ndev = aq_nic_ndev_alloc(); > + self = netdev_priv(ndev); > + if (!self) { For this and all other checks of self: how can self ever be NULL? > + > +int aq_nic_ndev_register(struct aq_nic_s *self) > +{ > + int err = 0; > + unsigned int i = 0U; > + > + if (!self->ndev) { > + err = -EINVAL; > + goto err_exit; > + } > + err = self->aq_hw_ops.hw_get_mac_permanent(self->aq_hw, > + self->aq_nic_cfg.aq_hw_caps, > + self->ndev->dev_addr); > + if (err < 0) > + goto err_exit; > + > +#if defined(AQ_CFG_MAC_ADDR_PERMANENT) > + { > + static u8 mac_addr_permanent[] = AQ_CFG_MAC_ADDR_PERMANENT; > + > + ether_addr_copy(self->ndev->dev_addr, mac_addr_permanent); > + } > +#endif > + err = register_netdev(self->ndev); > + if (err < 0) > + goto err_exit; > + This looks racy. Note that as soon as you call register_netdev() your drivers open() function can be called. You have to setup everything _before_ you call register_netdev(). > + self->is_ndev_registered = true; > + netif_carrier_off(self->ndev); > + > + for (i = AQ_CFG_VECS_MAX; i--;) > + aq_nic_ndev_queue_stop(self, i); > + > + > +static unsigned int aq_nic_map_skb_frag(struct aq_nic_s *self, > + struct sk_buff *skb, > + struct aq_ring_buff_s *dx) > +{ > + unsigned int ret = 0U; > + unsigned int nr_frags = skb_shinfo(skb)->nr_frags; > + unsigned int frag_count = 0U; > + > + dx->flags = 0U; > + dx->len = skb_headlen(skb); > + dx->pa = dma_map_single(aq_nic_get_dev(self), skb->data, dx->len, > + DMA_TO_DEVICE); Mapping can fail. You have to check the result. > + dx->len_pkt = skb->len; > + dx->is_sop = 1U; > + dx->is_mapped = 1U; > + > + ++ret; > + > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > + dx->is_ip_cso = (htons(ETH_P_IP) == skb->protocol) ? 1U : 0U; > + dx->is_tcp_cso = > + (ip_hdr(skb)->protocol == IPPROTO_TCP) ? 1U : 0U; > + dx->is_udp_cso = > + (ip_hdr(skb)->protocol == IPPROTO_UDP) ? 1U : 0U; > + } > + > + for (; nr_frags--; ++frag_count) { > + unsigned int frag_len; > + dma_addr_t frag_pa; > + skb_frag_t *frag = &skb_shinfo(skb)->frags[frag_count]; > + > + frag_len = skb_frag_size(frag); > + > + frag_pa = skb_frag_dma_map(aq_nic_get_dev(self), frag, 0, > + frag_len, DMA_TO_DEVICE); Same here, you have to check if mapping was successful. > + > +int aq_nic_xmit(struct aq_nic_s *self, struct sk_buff *skb) > +__releases(&ring->lock) > +__acquires(&ring->lock) > +{ > + struct aq_ring_s *ring = NULL; > + unsigned int frags = 0U; > + unsigned int vec = skb->queue_mapping % self->aq_nic_cfg.vecs; > + unsigned int tc = 0U; > + unsigned int trys = AQ_CFG_LOCK_TRYS; > + int err = 0; Please use NETDEV_TX_OK instead of 0 as the return value for successful transmission. > + bool is_nic_in_bad_state; > + bool is_busy = false; What is "is_busy" needed for? > + struct aq_ring_buff_s buffers[AQ_CFG_SKB_FRAGS_MAX]; You allocate quite large buffers on a very limited resource (the kernel stack) only to copy the data from the local buffers to the xmit buffer via aq_ring_tx_append_buffs(). Why dont map the xmit buffers directly? > + > + frags = skb_shinfo(skb)->nr_frags + 1; > + > + ring = self->aq_ring_tx[AQ_NIC_TCVEC2RING(self, tc, vec)]; > + > + atomic_inc(&self->header.busy_count); > + is_busy = true; > + > + if (frags > AQ_CFG_SKB_FRAGS_MAX) { > + dev_kfree_skb_any(skb); > + goto err_exit; > + } > + > + is_nic_in_bad_state = aq_utils_obj_test(&self->header.flags, > + AQ_NIC_FLAGS_IS_NOT_TX_READY) || > + (aq_ring_avail_dx(ring) < > + AQ_CFG_SKB_FRAGS_MAX); > + > + if (is_nic_in_bad_state) { > + aq_nic_ndev_queue_stop(self, ring->idx); > + err = NETDEV_TX_BUSY; > + goto err_exit; > + } > + > + do { > + if (spin_trylock(&ring->header.lock)) { > + frags = aq_nic_map_skb(self, skb, &buffers[0]); > + > + aq_ring_tx_append_buffs(ring, &buffers[0], frags); > + > + err = self->aq_hw_ops.hw_ring_tx_xmit(self->aq_hw, > + ring, frags); > + if (err >= 0) { > + if (aq_ring_avail_dx(ring) < > + AQ_CFG_SKB_FRAGS_MAX + 1) > + aq_nic_ndev_queue_stop(self, ring->idx); > + } > + spin_unlock(&ring->header.lock); > + > + if (err >= 0) { > + ++ring->stats.tx.packets; > + ring->stats.tx.bytes += skb->len; > + } > + break; > + } > + } while (--trys); AFAICS busy_count is only used in the timer callback. Why do they have to be synchronized? > + > +int aq_nic_stop(struct aq_nic_s *self) > +{ > + struct aq_vec_s *aq_vec = NULL; > + unsigned int i = 0U; > + > + for (i = 0U, aq_vec = self->aq_vec[0]; > + self->aq_vecs > i; ++i, aq_vec = self->aq_vec[i]) > + aq_nic_ndev_queue_stop(self, i); > + > + del_timer_sync(&self->service_timer); This is not sufficient. You also have to make sure that the timer callback does not restart the timer. > + > +int aq_nic_change_pm_state(struct aq_nic_s *self, pm_message_t *pm_msg) > +{ > + int err = 0; > + > + if (!netif_running(self->ndev)) { > + err = 0; > + goto err_exit; > + } > + rtnl_lock(); > + if (pm_msg->event & PM_EVENT_SLEEP || pm_msg->event & PM_EVENT_FREEZE) { > + self->power_state = AQ_HW_POWER_STATE_D3; > + netif_device_detach(self->ndev); > + netif_tx_stop_all_queues(self->ndev); > + > + err = aq_nic_stop(self); > + if (err < 0) > + goto err_exit; rtnl_unlock() is missing. > + > + aq_nic_deinit(self); > + } else { > + err = aq_nic_init(self); > + if (err < 0) > + goto err_exit; > + > + err = aq_nic_start(self); > + if (err < 0) > + goto err_exit; > + > + netif_device_attach(self->ndev); > + netif_tx_start_all_queues(self->ndev); > + } > + rtnl_unlock(); > + > +err_exit: > + return err; > +} Regards, Lino
Powered by blists - more mailing lists