Skip to contentSkip to conversation

ethtool: vclock: propagate errors from ptp_get_vclocks_index()

PATCH In review Jakub Kicinski 3 participants 4 messages May 26 – May 28

Subsystems (6)
Networking Network drivers Sockets Networking ethtool +2 more subsystems
lore
Share & tools
lore

Jakub Kicinski

author maintainer
#0 lore raw

PATCH 1/1 ethtool: vclock: propagate errors from ptp_get_vclocks_index()

ptp_get_vclocks_index() was written with sock_timestamping_bind_phc() in mind. Any return <= 0 will get converted to EINVAL by the virtue of sock_timestamping_bind_phc() not finding a matching vclock.

ethtool's phc_vclocks_prepare_data(), however, should not be silently returning empty messages on allocation errors or when PHC does not exist at all.

Commit c156174a6707 ("ethtool: add a new command for getting PHC virtual clocks") added phc_vclocks_prepare_data(), but let's not treat this as a fix in case some user space app now depends on the failures being silent.

The check in sock_timestamping_bind_phc() is not strickly necessary, but include it just for clarity.

Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 include/linux/ptp_clock_kernel.h |  4 ++--
 drivers/ptp/ptp_vclock.c         | 23 +++++++++++++----------
 net/core/sock.c                  |  2 ++
 net/ethtool/phc_vclocks.c        |  2 +-
 4 files changed, 18 insertions(+), 13 deletions(-)
include/linux/ptp_clock_kernel.h +2 -2
index 884364596dd3..a245989e13c1 100644
@@ -485,7 +485,7 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp)
  * @pclock_index: phc index of ptp pclock.
  * @vclock_index: pointer to pointer of vclock index.
  *
- * return number of vclocks.
+ * Returns: number of vclocks on success, or negative errno.
  */
 int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
 

@@ -500,7 +500,7 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
 ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
 #else
 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
-{ return 0; }
+{ return -ENODEV; }
 static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
 					    int vclock_index)
 { return 0; }
drivers/ptp/ptp_vclock.c +13 -10
index 915a4f6defc9..062860756640 100644
@@ -242,33 +242,36 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
 	char name[PTP_CLOCK_NAME_LEN] = "";
 	struct ptp_clock *ptp;
 	struct device *dev;
-	int num = 0;
+	int ret;
 
 	if (pclock_index < 0)
-		return num;
+		return -ENODEV;
 
 	snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", pclock_index);
 	dev = class_find_device_by_name(&ptp_class, name);
 	if (!dev)
-		return num;
+		return -ENODEV;
 
 	ptp = dev_get_drvdata(dev);
 
 	if (mutex_lock_interruptible(&ptp->n_vclocks_mux)) {
-		put_device(dev);
-		return num;
+		ret = -EINTR;
+		goto exit_put_dev;
 	}
 
 	*vclock_index = kzalloc(sizeof(int) * ptp->n_vclocks, GFP_KERNEL);
-	if (!(*vclock_index))
-		goto out;
+	if (!(*vclock_index)) {
+		ret = -ENOMEM;
+		goto exit_unlock;
+	}
 
 	memcpy(*vclock_index, ptp->vclock_index, sizeof(int) * ptp->n_vclocks);
-	num = ptp->n_vclocks;
-out:
+	ret = ptp->n_vclocks;
+exit_unlock:
 	mutex_unlock(&ptp->n_vclocks_mux);
+exit_put_dev:
 	put_device(dev);
-	return num;
+	return ret;
 }
 EXPORT_SYMBOL(ptp_get_vclocks_index);
net/core/sock.c +2 -0
index b37b664b6eb9..9e33e979a283 100644
@@ -870,6 +870,8 @@ static int sock_timestamping_bind_phc(struct sock *sk, int phc_index)
 
 	num = ethtool_get_phc_vclocks(dev, &vclock_index);
 	dev_put(dev);
+	if (num < 0)
+		return num;
 
 	for (i = 0; i < num; i++) {
 		if (*(vclock_index + i) == phc_index) {
net/ethtool/phc_vclocks.c +1 -1
index 15146e38ab27..eec5a08f950b 100644
@@ -36,7 +36,7 @@ static int phc_vclocks_prepare_data(const struct ethnl_req_info *req_base,
 	data->num = ethtool_get_phc_vclocks(dev, &data->index);
 	ethnl_ops_complete(dev);
 
-	return ret;
+	return data->num < 0 ? data->num : 0;
 }
 
 static int phc_vclocks_reply_size(const struct ethnl_req_info *req_base,
Signature · 1 line
2.54.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 8 lines · Jakub Kicinski

ptp_get_vclocks_index() was written with sock_timestamping_bind_phc() in mind. Any return <= 0 will get converted to EINVAL by the virtue of sock_timestamping_bind_phc() not finding a matching vclock.

ethtool's phc_vclocks_prepare_data(), however, should not be silently returning empty messages on allocation errors or when PHC does not exist at all.

Quoted 6 lines

Commit c156174a6707 ("ethtool: add a new command for getting PHC virtual clocks") added phc_vclocks_prepare_data(), but let's not treat this as a fix in case some user space app now depends on the failures being silent.

The check in sock_timestamping_bind_phc() is not strickly necessary,

strictly

Quoted 3 lines

but include it just for clarity.

Signed-off-by Jakub Kicinski <[email protected]>
Reviewed-by Willem de Bruijn <[email protected]>
Quoted 6 lines
net/core/sock.c +0 -0
index b37b664b6eb9..9e33e979a283 100644

net/core/sock.c static int sock_timestamping_bind_phc(struct sock *sk, int phc_index) · line 877

Quoted 8 lines
net/core/sock.c +2 -0
@@ -870,6 +870,8 @@ static int sock_timestamping_bind_phc(struct sock *sk, int phc_index)
 
 	num = ethtool_get_phc_vclocks(dev, &vclock_index);
 	dev_put(dev);
+	if (num < 0)
+		return num;
 
 	for (i = 0; i < num; i++) {
 		if (*(vclock_index + i) == phc_index) {

Not for this patch:

This is called by sk_setsockopt and by MPTCP for every subflow. Apparently MPTCP does not check the return value. Should it?

#2 lore raw

Hi Willem,

Thank you for the Cc!

Quoted 13 lines · Willem de Bruijn
Quoted 12 lines · Jakub Kicinski

ptp_get_vclocks_index() was written with sock_timestamping_bind_phc() in mind. Any return <= 0 will get converted to EINVAL by the virtue of sock_timestamping_bind_phc() not finding a matching vclock.

ethtool's phc_vclocks_prepare_data(), however, should not be silently returning empty messages on allocation errors or when PHC does not exist at all.

Commit c156174a6707 ("ethtool: add a new command for getting PHC virtual clocks") added phc_vclocks_prepare_data(), but let's not treat this as a fix in case some user space app now depends on the failures being silent.

(...)

Quoted 11 lines
Quoted 11 lines
net/core/sock.c +2 -0
index b37b664b6eb9..9e33e979a283 100644
@@ -870,6 +870,8 @@ static int sock_timestamping_bind_phc(struct sock *sk, int phc_index)
 
 	num = ethtool_get_phc_vclocks(dev, &vclock_index);
 	dev_put(dev);
+	if (num < 0)
+		return num;
Quoted 7 lines

net/core/sock.c static int sock_timestamping_bind_phc(struct sock *sk, int phc_index) · line 877

Quoted 2 lines
net/core/sock.c +0 -0
 	for (i = 0; i < num; i++) {
 		if (*(vclock_index + i) == phc_index) {

Not for this patch:

This is called by sk_setsockopt and by MPTCP for every subflow. Apparently MPTCP does not check the return value. Should it?

Good point! I guess there was a confusion between sock_set_timestamp() (SO_TIMESTAMPNS) and sock_set_timestamping() (SO_TIMESTAMPING).

I can send a patch to fix that.

Cheers,
Matt

Signature · 1 line
Sponsored by the NGI0 Core fund.
author maintainer
#3 lore raw
In reply to Jakub Kicinski
Quoted 3 lines · On Tue, 26 May 2026 08:36:50 -0700 Jakub Kicinski wrote:

ptp_get_vclocks_index() was written with sock_timestamping_bind_phc() in mind. Any return <= 0 will get converted to EINVAL by the virtue of sock_timestamping_bind_phc() not finding a matching vclock.

Sashiko points out that dump needs to skip over devices without vclock cleanly. I'll respin.

Signature · 1 line
pw-bot: cr