Re: [PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()
On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote: > This patch converts some basic cases of ethtool_sprintf() to > ethtool_puts(). > > The conversions are used in cases where ethtool_sprintf() was being used > with just two arguments: > > ethtool_sprintf(&data, buffer[i].name); OK. > or when it's used with format string: "%s" > > ethtool_sprintf(&data, "%s", buffer[i].name); > > which both now become: > > ethtool_puts(&data, buffer[i].name); Why do you want this conversion? Is it not possible for .name to contain a formatting field?
Re: [PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()
On Wed, Oct 25, 2023 at 4:51 PM Joe Perches wrote: > > On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote: > > This patch converts some basic cases of ethtool_sprintf() to > > ethtool_puts(). > > > > The conversions are used in cases where ethtool_sprintf() was being used > > with just two arguments: > > > ethtool_sprintf(&data, buffer[i].name); > > OK. > > > or when it's used with format string: "%s" > > > ethtool_sprintf(&data, "%s", buffer[i].name); > > > which both now become: > > > ethtool_puts(&data, buffer[i].name); > > Why do you want this conversion? > Is it not possible for .name to contain a formatting field? The case of using just two arguments to a ethtool_sprintf call may cause -Wformat-security warnings. If it did indeed have format specifiers then we would have more format specifiers than arguments. Not ideal. The second case of having a standalone "%s" isn't necessarily bad or wrong. I used this exact approach to replace some strncpy() usage in net drivers [1]. I'm working off guidance from Andrew Lunn [2] and Kees who said it may be a good idea to tidy this up with a puts(). All in all, this patch doesn't do much but fix some warnings and provide a more obvious interface. The number of actual replacements are relatively low (around 20ish) so I was hoping to sneak them in via this series. > [1]: https://lore.kernel.org/all/?q=dfb%3Aethtool_sprintf+AND+f%3Ajustinstitt [2]: https://lore.kernel.org/all/a958d35e-98b6-4a95-b505-776482d11...@lunn.ch/ Thanks Justin
Re: [PATCH 3/3] checkpatch: add ethtool_sprintf rules
On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote: > Add some warnings for using ethtool_sprintf() where a simple > ethtool_puts() would suffice. > > The two cases are: > > 1) Use ethtool_sprintf() with just two arguments: > > ethtool_sprintf(&data, driver[i].name); OK. > or > 2) Use ethtool_sprintf() with a standalone "%s" fmt string: > > ethtool_sprintf(&data, "%s", driver[i].name); I'm rather doubt this is really desired or appropriate.
[PATCH 3/3] checkpatch: add ethtool_sprintf rules
Add some warnings for using ethtool_sprintf() where a simple
ethtool_puts() would suffice.
The two cases are:
1) Use ethtool_sprintf() with just two arguments:
| ethtool_sprintf(&data, driver[i].name);
or
2) Use ethtool_sprintf() with a standalone "%s" fmt string:
| ethtool_sprintf(&data, "%s", driver[i].name);
The former may cause -Wformat-security warnings while the latter is just
not preferred. Both are safely in the category of warnings, not errors.
Signed-off-by: Justin Stitt
---
scripts/checkpatch.pl | 13 +
1 file changed, 13 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7d16f863edf1..1ba9ce778746 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7020,6 +7020,19 @@ sub process {
"Prefer strscpy, strscpy_pad, or __nonstring over
strncpy - see: https://github.com/KSPP/linux/issues/90\n"; . $herecurr);
}
+# ethtool_sprintf uses that should likely be ethtool_puts
+ if ( $line =~
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/ ) {
+ WARN("ETHTOOL_SPRINTF",
+"Prefer ethtool_puts over ethtool_sprintf with
only two arguments" . $herecurr);
+ }
+
+ # use $rawline because $line loses %s via sanitization and thus
we can't match against it.
+ if ( $rawline =~
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/ ) {
+ WARN("ETHTOOL_SPRINTF2",
+"Prefer ethtool_puts over ethtool_sprintf with
standalone \"%s\" specifier" . $herecurr);
+ }
+
+
# typecasts on min/max could be min_t/max_t
if ($perl_version_ok &&
defined $stat &&
--
2.42.0.758.gaed0368e0e-goog
[PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()
This patch converts some basic cases of ethtool_sprintf() to
ethtool_puts().
The conversions are used in cases where ethtool_sprintf() was being used
with just two arguments:
| ethtool_sprintf(&data, buffer[i].name);
or when it's used with format string: "%s"
| ethtool_sprintf(&data, "%s", buffer[i].name);
which both now become:
| ethtool_puts(&data, buffer[i].name);
There are some outstanding patches [1] that I've sent using plain "%s"
with ethtool_sprintf() that should be ethtool_puts() now. Some have been
picked up as-is but I will send new versions for the others.
[1]: https://lore.kernel.org/all/?q=dfb%3Aethtool_sprintf+AND+f%3Ajustinstitt
Signed-off-by: Justin Stitt
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 +-
drivers/net/ethernet/brocade/bna/bnad_ethtool.c| 2 +-
.../net/ethernet/fungible/funeth/funeth_ethtool.c | 8 +--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 2 +-
.../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c| 2 +-
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 66 +++---
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 4 +-
drivers/net/ethernet/intel/ice/ice_ethtool.c | 10 ++--
drivers/net/ethernet/intel/igb/igb_ethtool.c | 6 +-
drivers/net/ethernet/intel/igc/igc_ethtool.c | 6 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 5 +-
.../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 44 +++
drivers/net/ethernet/pensando/ionic/ionic_stats.c | 4 +-
drivers/net/hyperv/netvsc_drv.c| 4 +-
drivers/net/vmxnet3/vmxnet3_ethtool.c | 10 ++--
15 files changed, 87 insertions(+), 90 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index d671df4b76bc..e3ef081aa42b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -299,13 +299,13 @@ static void ena_get_strings(struct ena_adapter *adapter,
for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) {
ena_stats = &ena_stats_global_strings[i];
- ethtool_sprintf(&data, ena_stats->name);
+ ethtool_puts(&data, ena_stats->name);
}
if (eni_stats_needed) {
for (i = 0; i < ENA_STATS_ARRAY_ENI(adapter); i++) {
ena_stats = &ena_stats_eni_strings[i];
- ethtool_sprintf(&data, ena_stats->name);
+ ethtool_puts(&data, ena_stats->name);
}
}
diff --git a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
index df10edff5603..d1ad6c9f8140 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
@@ -608,7 +608,7 @@ bnad_get_strings(struct net_device *netdev, u32 stringset,
u8 *string)
for (i = 0; i < BNAD_ETHTOOL_STATS_NUM; i++) {
BUG_ON(!(strlen(bnad_net_stats_strings[i]) < ETH_GSTRING_LEN));
- ethtool_sprintf(&string, bnad_net_stats_strings[i]);
+ ethtool_puts(&string, bnad_net_stats_strings[i]);
}
bmap = bna_tx_rid_mask(&bnad->bna);
diff --git a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
index 31aa185f4d17..091c93bd7587 100644
--- a/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
+++ b/drivers/net/ethernet/fungible/funeth/funeth_ethtool.c
@@ -655,7 +655,7 @@ static void fun_get_strings(struct net_device *netdev, u32
sset, u8 *data)
i);
}
for (j = 0; j < ARRAY_SIZE(txq_stat_names); j++)
- ethtool_sprintf(&p, txq_stat_names[j]);
+ ethtool_puts(&p, txq_stat_names[j]);
for (i = 0; i < fp->num_xdpqs; i++) {
for (j = 0; j < ARRAY_SIZE(xdpq_stat_names); j++)
@@ -663,7 +663,7 @@ static void fun_get_strings(struct net_device *netdev, u32
sset, u8 *data)
xdpq_stat_names[j], i);
}
for (j = 0; j < ARRAY_SIZE(xdpq_stat_names); j++)
- ethtool_sprintf(&p, xdpq_stat_names[j]);
+ ethtool_puts(&p, xdpq_stat_names[j]);
for (i = 0; i < netdev->real_num_rx_queues; i++) {
for (j = 0; j < ARRAY_SIZE(rxq_stat_names); j++)
@@ -671,10 +671,10 @@ static void fun_get_strings(struct net_device *netdev,
u32 sset, u8 *data)
i);
}
for (j = 0; j < ARRAY_SIZE(rxq_stat_names); j++)
- ethtool_sprintf(&p, rxq_stat_names[j]);
+ ethtool_puts(&p, rxq_stat_names[j]);
for (j = 0; j < ARRAY_SIZE(tls_stat_names); j++)
-
[PATCH 1/3] ethtool: Implement ethtool_puts()
Use strscpy() to implement ethtool_puts().
Functionally the same as ethtool_sprintf() when it's used with two
arguments or with just "%s" format specifier.
Signed-off-by: Justin Stitt
---
include/linux/ethtool.h | 13 +
net/ethtool/ioctl.c | 7 +++
2 files changed, 20 insertions(+)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 62b61527bcc4..fdd65050bf1b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1052,4 +1052,17 @@ static inline int ethtool_mm_frag_size_min_to_add(u32
val_min, u32 *val_add,
* next string.
*/
extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
+
+/**
+ * ethtool_puts - Write string to ethtool string data
+ * @data: Pointer to start of string to update
+ * @str: String to write
+ *
+ * Write string to data. Update data to point at start of next
+ * string.
+ *
+ * Prefer this function to ethtool_sprintf() when given only
+ * two arguments or if @fmt is just "%s".
+ */
+extern void ethtool_puts(u8 **data, const char *str);
#endif /* _LINUX_ETHTOOL_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..abdf05edf804 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1991,6 +1991,13 @@ __printf(2, 3) void ethtool_sprintf(u8 **data, const
char *fmt, ...)
}
EXPORT_SYMBOL(ethtool_sprintf);
+void ethtool_puts(u8 **data, const char *str)
+{
+ strscpy(*data, str, ETH_GSTRING_LEN);
+ *data += ETH_GSTRING_LEN;
+}
+EXPORT_SYMBOL(ethtool_puts);
+
static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
{
struct ethtool_value id;
--
2.42.0.758.gaed0368e0e-goog
[PATCH 0/3] ethtool: Add ethtool_puts()
Hi, This series aims to implement ethtool_puts() and send out a wave 1 of conversions from ethtool_sprintf(). There's also a checkpatch patch included to check for the cases listed below. This was sparked from recent discussion here [1] The conversions are used in cases where ethtool_sprintf() was being used with just two arguments: | ethtool_sprintf(&data, buffer[i].name); or when it's used with format string: "%s" | ethtool_sprintf(&data, "%s", buffer[i].name); which both now become: | ethtool_puts(&data, buffer[i].name); The first case commonly triggers a -Wformat-security warning with Clang due to potential problems with format flags present in the strings [3]. The second is just a bit weird with a plain-ol' "%s". Note that I have some outstanding patches [2] (some picked up) that use the second case of ethtool_sprintf(). I went with this approach to clean up some strncpy() uses and avoid -Wformat-security warnings before discussion about implementing ...puts() arose. I will probably let the ones that have been picked up land but send new versions for others. Wave 1 changes found with Cocci [4] and grep [5]. [1]: https://lore.kernel.org/all/202310141935.B326C9E@keescook/ [2]: https://lore.kernel.org/all/?q=dfb%3Aethtool_sprintf+AND+f%3Ajustinstitt [3]: https://lore.kernel.org/all/202310101528.9496539BE@keescook/ [4]: (script authored by Kees) @replace_2_args@ identifier BUF; expression VAR; @@ - ethtool_sprintf + ethtool_puts (&BUF, VAR) @replace_3_args@ identifier BUF; expression VAR; @@ - ethtool_sprintf(&BUF, "%s", VAR) + ethtool_puts(&BUF, VAR) [5]: $ rg "ethtool_sprintf\(\s*[^,)]+\s*,\s*[^,)]+\s*\)" Signed-off-by: Justin Stitt --- Justin Stitt (3): ethtool: Implement ethtool_puts() treewide: Convert some ethtool_sprintf() to ethtool_puts() checkpatch: add ethtool_sprintf rules drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 +- drivers/net/ethernet/brocade/bna/bnad_ethtool.c| 2 +- .../net/ethernet/fungible/funeth/funeth_ethtool.c | 8 +-- drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 2 +- .../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c| 2 +- drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 66 +++--- drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 4 +- drivers/net/ethernet/intel/ice/ice_ethtool.c | 10 ++-- drivers/net/ethernet/intel/igb/igb_ethtool.c | 6 +- drivers/net/ethernet/intel/igc/igc_ethtool.c | 6 +- drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 5 +- .../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 44 +++ drivers/net/ethernet/pensando/ionic/ionic_stats.c | 4 +- drivers/net/hyperv/netvsc_drv.c| 4 +- drivers/net/vmxnet3/vmxnet3_ethtool.c | 10 ++-- include/linux/ethtool.h| 13 + net/ethtool/ioctl.c| 7 +++ scripts/checkpatch.pl | 13 + 18 files changed, 120 insertions(+), 90 deletions(-) --- base-commit: d88520ad73b79e71e3ddf08de335b8520ae41c5c change-id: 20231025-ethtool_puts_impl-a1479ffbc7e0 Best regards, -- Justin Stitt
[PATCH] hv_netvsc: Mark VF as slave before exposing it to user-mode
From: Long Li
When a VF is being exposed form the kernel, it should be marked as "slave"
before exposing to the user-mode. The VF is not usable without netvsc running
as master. The user-mode should never see a VF without the "slave" flag.
This commit moves the code of setting the slave flag to the time before VF is
exposed to user-mode.
Signed-off-by: Long Li
---
drivers/net/hyperv/netvsc_drv.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index ec77fb9dcf89..045777a4971d 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2206,9 +2206,6 @@ static int netvsc_vf_join(struct net_device *vf_netdev,
goto upper_link_failed;
}
- /* set slave flag before open to prevent IPv6 addrconf */
- vf_netdev->flags |= IFF_SLAVE;
-
schedule_delayed_work(&ndev_ctx->vf_takeover, VF_TAKEOVER_INT);
call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
@@ -2320,11 +2317,9 @@ static struct net_device *get_netvsc_byslot(const struct
net_device *vf_netdev)
*/
list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
ndev = hv_get_drvdata(ndev_ctx->device_ctx);
- if (ether_addr_equal(vf_netdev->perm_addr, ndev->perm_addr)) {
- netdev_notice(vf_netdev,
- "falling back to mac addr based
matching\n");
+ if (ether_addr_equal(vf_netdev->perm_addr, ndev->perm_addr) ||
+ ether_addr_equal(vf_netdev->dev_addr, ndev->perm_addr))
return ndev;
- }
}
netdev_notice(vf_netdev,
@@ -2332,7 +2327,7 @@ static struct net_device *get_netvsc_byslot(const struct
net_device *vf_netdev)
return NULL;
}
-static int netvsc_register_vf(struct net_device *vf_netdev)
+static int netvsc_register_vf(struct net_device *vf_netdev, unsigned long
event)
{
struct net_device_context *net_device_ctx;
struct netvsc_device *netvsc_dev;
@@ -2347,6 +2342,12 @@ static int netvsc_register_vf(struct net_device
*vf_netdev)
if (!ndev)
return NOTIFY_DONE;
+ if (event == NETDEV_POST_INIT) {
+ /* set slave flag before open to prevent IPv6 addrconf */
+ vf_netdev->flags |= IFF_SLAVE;
+ return NOTIFY_DONE;
+ }
+
net_device_ctx = netdev_priv(ndev);
netvsc_dev = rtnl_dereference(net_device_ctx->nvdev);
if (!netvsc_dev || rtnl_dereference(net_device_ctx->vf_netdev))
@@ -2753,8 +2754,9 @@ static int netvsc_netdev_event(struct notifier_block
*this,
return NOTIFY_DONE;
switch (event) {
+ case NETDEV_POST_INIT:
case NETDEV_REGISTER:
- return netvsc_register_vf(event_dev);
+ return netvsc_register_vf(event_dev, event);
case NETDEV_UNREGISTER:
return netvsc_unregister_vf(event_dev);
case NETDEV_UP:
--
2.34.1
[PATCH net] hv_netvsc: fix race of netvsc and VF register_netdevice
The rtnl lock also needs to be held before rndis_filter_device_add()
which advertises nvsp_2_vsc_capability / sriov bit, and triggers
VF NIC offering and registering. If VF NIC finished register_netdev()
earlier it may cause name based config failure.
To fix this issue, move the call to rtnl_lock() before
rndis_filter_device_add(), so VF will be registered later than netvsc
/ synthetic NIC, and gets a name numbered (ethX) after netvsc.
And, move register_netdevice_notifier() earlier, so the call back
function is set before probing.
Cc: sta...@vger.kernel.org
Fixes: e04e7a7bbd4b ("hv_netvsc: Fix a deadlock by getting rtnl lock earlier in
netvsc_probe()")
Signed-off-by: Haiyang Zhang
---
drivers/net/hyperv/netvsc_drv.c | 30 +++---
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3ba3c8fb28a5..feca1391f756 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -2531,15 +2531,6 @@ static int netvsc_probe(struct hv_device *dev,
goto devinfo_failed;
}
- nvdev = rndis_filter_device_add(dev, device_info);
- if (IS_ERR(nvdev)) {
- ret = PTR_ERR(nvdev);
- netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
- goto rndis_failed;
- }
-
- eth_hw_addr_set(net, device_info->mac_adr);
-
/* We must get rtnl lock before scheduling nvdev->subchan_work,
* otherwise netvsc_subchan_work() can get rtnl lock first and wait
* all subchannels to show up, but that may not happen because
@@ -2547,9 +2538,23 @@ static int netvsc_probe(struct hv_device *dev,
* -> ... -> device_add() -> ... -> __device_attach() can't get
* the device lock, so all the subchannels can't be processed --
* finally netvsc_subchan_work() hangs forever.
+*
+* The rtnl lock also needs to be held before rndis_filter_device_add()
+* which advertises nvsp_2_vsc_capability / sriov bit, and triggers
+* VF NIC offering and registering. If VF NIC finished register_netdev()
+* earlier it may cause name based config failure.
*/
rtnl_lock();
+ nvdev = rndis_filter_device_add(dev, device_info);
+ if (IS_ERR(nvdev)) {
+ ret = PTR_ERR(nvdev);
+ netdev_err(net, "unable to add netvsc device (ret %d)\n", ret);
+ goto rndis_failed;
+ }
+
+ eth_hw_addr_set(net, device_info->mac_adr);
+
if (nvdev->num_chn > 1)
schedule_work(&nvdev->subchan_work);
@@ -2788,11 +2793,14 @@ static int __init netvsc_drv_init(void)
}
netvsc_ring_bytes = ring_size * PAGE_SIZE;
+ register_netdevice_notifier(&netvsc_netdev_notifier);
+
ret = vmbus_driver_register(&netvsc_drv);
- if (ret)
+ if (ret) {
+ unregister_netdevice_notifier(&netvsc_netdev_notifier);
return ret;
+ }
- register_netdevice_notifier(&netvsc_netdev_notifier);
return 0;
}
--
2.25.1
RE: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp
> -Original Message- > From: Jason Gunthorpe > Sent: Monday, October 23, 2023 11:24 AM > To: sharmaa...@linuxonhyperv.com > Cc: Long Li ; Leon Romanovsky ; > Dexuan Cui ; Wei Liu ; David S. > Miller ; Eric Dumazet ; > Jakub Kicinski ; Paolo Abeni ; linux- > r...@vger.kernel.org; linux-hyperv@vger.kernel.org; > net...@vger.kernel.org; linux-ker...@vger.kernel.org; Ajay Sharma > > Subject: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp > > On Mon, Oct 16, 2023 at 03:12:02PM -0700, > sharmaa...@linuxonhyperv.com wrote: > > > diff --git a/drivers/infiniband/hw/mana/qp.c > > b/drivers/infiniband/hw/mana/qp.c index ef3275ac92a0..19fae28985c3 > > 100644 > > --- a/drivers/infiniband/hw/mana/qp.c > > +++ b/drivers/infiniband/hw/mana/qp.c > > @@ -210,6 +210,8 @@ static int mana_ib_create_qp_rss(struct ib_qp > *ibqp, struct ib_pd *pd, > > wq->id = wq_spec.queue_index; > > cq->id = cq_spec.queue_index; > > > > + xa_store(&mib_dev->rq_to_qp_lookup_table, wq->id, qp, > GFP_KERNEL); > > + > > A store with no erase? > > A load with no locking? > > This can't be right > > Jason This wq->id is assigned from the HW and is guaranteed to be unique. May be I am not following why do we need a lock here. Can you please explain ? Ajay
