For review, the following three patches address bugs in the Linux
NFS/RDMA server implementation.
---
Chuck Lever (3):
svcrdma: Fix trace point use-after-free race
SUNRPC: Remove naked ->xpo_release_rqst from svc_send()
svcrdma: Fix leak of svc_rdma_recv_ctxt objects
include/linux/sunrpc/svc_rdma.h | 1 +
net/sunrpc/svc_xprt.c | 3 ---
net/sunrpc/svcsock.c | 4 ++++
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++
net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++----------
net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 -----
6 files changed, 30 insertions(+), 18 deletions(-)
--
Chuck Lever
I hit this while testing nfsd-5.7 with kernel memory debugging
enabled on my server:
Mar 30 13:21:45 klimt kernel: BUG: unable to handle page fault for address: ffff8887e6c279a8
Mar 30 13:21:45 klimt kernel: #PF: supervisor read access in kernel mode
Mar 30 13:21:45 klimt kernel: #PF: error_code(0x0000) - not-present page
Mar 30 13:21:45 klimt kernel: PGD 3601067 P4D 3601067 PUD 87c519067 PMD 87c3e2067 PTE 800ffff8193d8060
Mar 30 13:21:45 klimt kernel: Oops: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
Mar 30 13:21:45 klimt kernel: CPU: 2 PID: 1933 Comm: nfsd Not tainted 5.6.0-rc6-00040-g881e87a3c6f9 #1591
Mar 30 13:21:45 klimt kernel: Hardware name: Supermicro Super Server/X10SRL-F, BIOS 1.0c 09/09/2015
Mar 30 13:21:45 klimt kernel: RIP: 0010:svc_rdma_post_chunk_ctxt+0xab/0x284 [rpcrdma]
Mar 30 13:21:45 klimt kernel: Code: c1 83 34 02 00 00 29 d0 85 c0 7e 72 48 8b bb a0 02 00 00 48 8d 54 24 08 4c 89 e6 48 8b 07 48 8b 40 20 e8 5a 5c 2b e1 41 89 c6 <8b> 45 20 89 44 24 04 8b 05 02 e9 01 00 85 c0 7e 33 e9 5e 01 00 00
Mar 30 13:21:45 klimt kernel: RSP: 0018:ffffc90000dfbdd8 EFLAGS: 00010286
Mar 30 13:21:45 klimt kernel: RAX: 0000000000000000 RBX: ffff8887db8db400 RCX: 0000000000000030
Mar 30 13:21:45 klimt kernel: RDX: 0000000000000040 RSI: 0000000000000000 RDI: 0000000000000246
Mar 30 13:21:45 klimt kernel: RBP: ffff8887e6c27988 R08: 0000000000000000 R09: 0000000000000004
Mar 30 13:21:45 klimt kernel: R10: ffffc90000dfbdd8 R11: 00c068ef00000000 R12: ffff8887eb4e4a80
Mar 30 13:21:45 klimt kernel: R13: ffff8887db8db634 R14: 0000000000000000 R15: ffff8887fc931000
Mar 30 13:21:45 klimt kernel: FS: 0000000000000000(0000) GS:ffff88885bd00000(0000) knlGS:0000000000000000
Mar 30 13:21:45 klimt kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Mar 30 13:21:45 klimt kernel: CR2: ffff8887e6c279a8 CR3: 000000081b72e002 CR4: 00000000001606e0
Mar 30 13:21:45 klimt kernel: Call Trace:
Mar 30 13:21:45 klimt kernel: ? svc_rdma_vec_to_sg+0x7f/0x7f [rpcrdma]
Mar 30 13:21:45 klimt kernel: svc_rdma_send_write_chunk+0x59/0xce [rpcrdma]
Mar 30 13:21:45 klimt kernel: svc_rdma_sendto+0xf9/0x3ae [rpcrdma]
Mar 30 13:21:45 klimt kernel: ? nfsd_destroy+0x51/0x51 [nfsd]
Mar 30 13:21:45 klimt kernel: svc_send+0x105/0x1e3 [sunrpc]
Mar 30 13:21:45 klimt kernel: nfsd+0xf2/0x149 [nfsd]
Mar 30 13:21:45 klimt kernel: kthread+0xf6/0xfb
Mar 30 13:21:45 klimt kernel: ? kthread_queue_delayed_work+0x74/0x74
Mar 30 13:21:45 klimt kernel: ret_from_fork+0x3a/0x50
Mar 30 13:21:45 klimt kernel: Modules linked in: ocfs2_dlmfs ocfs2_stack_o2cb ocfs2_dlm ocfs2_nodemanager ocfs2_stackglue ib_umad ib_ipoib mlx4_ib sb_edac x86_pkg_temp_thermal iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel glue_helper crypto_simd cryptd pcspkr rpcrdma i2c_i801 rdma_ucm lpc_ich mfd_core ib_iser rdma_cm iw_cm ib_cm mei_me raid0 libiscsi mei sg scsi_transport_iscsi ioatdma wmi ipmi_si ipmi_devintf ipmi_msghandler acpi_power_meter nfsd nfs_acl lockd auth_rpcgss grace sunrpc ip_tables xfs libcrc32c mlx4_en sd_mod sr_mod cdrom mlx4_core crc32c_intel igb nvme i2c_algo_bit ahci i2c_core libahci nvme_core dca libata t10_pi qedr dm_mirror dm_region_hash dm_log dm_mod dax qede qed crc8 ib_uverbs ib_core
Mar 30 13:21:45 klimt kernel: CR2: ffff8887e6c279a8
Mar 30 13:21:45 klimt kernel: ---[ end trace 87971d2ad3429424 ]---
It's absolutely not safe to use resources pointed to by the @send_wr
argument of ib_post_send() _after_ that function returns. Those
resources are typically freed by the Send completion handler, which
can run before ib_post_send() returns.
Thus the trace points currently around ib_post_send() in the
server's RPC/RDMA transport are a hazard, even when they are
disabled. Rearrange them so that they touch the Work Request only
_before_ ib_post_send() is invoked.
Fixes: bd2abef33394 ("svcrdma: Trace key RDMA API events")
Fixes: 4201c7464753 ("svcrdma: Introduce svc_rdma_send_ctxt")
Signed-off-by: Chuck Lever <[email protected]>
---
include/trace/events/rpcrdma.h | 50 ++++++++++++++++++++++++---------
net/sunrpc/xprtrdma/svc_rdma_rw.c | 3 +-
net/sunrpc/xprtrdma/svc_rdma_sendto.c | 16 ++++++-----
3 files changed, 46 insertions(+), 23 deletions(-)
diff --git a/include/trace/events/rpcrdma.h b/include/trace/events/rpcrdma.h
index 9238d233f8cf..94087219f8a7 100644
--- a/include/trace/events/rpcrdma.h
+++ b/include/trace/events/rpcrdma.h
@@ -1722,17 +1722,15 @@ DECLARE_EVENT_CLASS(svcrdma_sendcomp_event,
TRACE_EVENT(svcrdma_post_send,
TP_PROTO(
- const struct ib_send_wr *wr,
- int status
+ const struct ib_send_wr *wr
),
- TP_ARGS(wr, status),
+ TP_ARGS(wr),
TP_STRUCT__entry(
__field(const void *, cqe)
__field(unsigned int, num_sge)
__field(u32, inv_rkey)
- __field(int, status)
),
TP_fast_assign(
@@ -1740,12 +1738,11 @@ TRACE_EVENT(svcrdma_post_send,
__entry->num_sge = wr->num_sge;
__entry->inv_rkey = (wr->opcode == IB_WR_SEND_WITH_INV) ?
wr->ex.invalidate_rkey : 0;
- __entry->status = status;
),
- TP_printk("cqe=%p num_sge=%u inv_rkey=0x%08x status=%d",
+ TP_printk("cqe=%p num_sge=%u inv_rkey=0x%08x",
__entry->cqe, __entry->num_sge,
- __entry->inv_rkey, __entry->status
+ __entry->inv_rkey
)
);
@@ -1810,26 +1807,23 @@ TRACE_EVENT(svcrdma_wc_receive,
TRACE_EVENT(svcrdma_post_rw,
TP_PROTO(
const void *cqe,
- int sqecount,
- int status
+ int sqecount
),
- TP_ARGS(cqe, sqecount, status),
+ TP_ARGS(cqe, sqecount),
TP_STRUCT__entry(
__field(const void *, cqe)
__field(int, sqecount)
- __field(int, status)
),
TP_fast_assign(
__entry->cqe = cqe;
__entry->sqecount = sqecount;
- __entry->status = status;
),
- TP_printk("cqe=%p sqecount=%d status=%d",
- __entry->cqe, __entry->sqecount, __entry->status
+ TP_printk("cqe=%p sqecount=%d",
+ __entry->cqe, __entry->sqecount
)
);
@@ -1897,6 +1891,34 @@ DECLARE_EVENT_CLASS(svcrdma_sendqueue_event,
DEFINE_SQ_EVENT(full);
DEFINE_SQ_EVENT(retry);
+TRACE_EVENT(svcrdma_sq_post_err,
+ TP_PROTO(
+ const struct svcxprt_rdma *rdma,
+ int status
+ ),
+
+ TP_ARGS(rdma, status),
+
+ TP_STRUCT__entry(
+ __field(int, avail)
+ __field(int, depth)
+ __field(int, status)
+ __string(addr, rdma->sc_xprt.xpt_remotebuf)
+ ),
+
+ TP_fast_assign(
+ __entry->avail = atomic_read(&rdma->sc_sq_avail);
+ __entry->depth = rdma->sc_sq_depth;
+ __entry->status = status;
+ __assign_str(addr, rdma->sc_xprt.xpt_remotebuf);
+ ),
+
+ TP_printk("addr=%s sc_sq_avail=%d/%d status=%d",
+ __get_str(addr), __entry->avail, __entry->depth,
+ __entry->status
+ )
+);
+
#endif /* _TRACE_RPCRDMA_H */
#include <trace/define_trace.h>
diff --git a/net/sunrpc/xprtrdma/svc_rdma_rw.c b/net/sunrpc/xprtrdma/svc_rdma_rw.c
index bd7c195d872e..23c2d3ce0dc9 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_rw.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_rw.c
@@ -323,8 +323,6 @@ static int svc_rdma_post_chunk_ctxt(struct svc_rdma_chunk_ctxt *cc)
if (atomic_sub_return(cc->cc_sqecount,
&rdma->sc_sq_avail) > 0) {
ret = ib_post_send(rdma->sc_qp, first_wr, &bad_wr);
- trace_svcrdma_post_rw(&cc->cc_cqe,
- cc->cc_sqecount, ret);
if (ret)
break;
return 0;
@@ -337,6 +335,7 @@ static int svc_rdma_post_chunk_ctxt(struct svc_rdma_chunk_ctxt *cc)
trace_svcrdma_sq_retry(rdma);
} while (1);
+ trace_svcrdma_sq_post_err(rdma, ret);
set_bit(XPT_CLOSE, &xprt->xpt_flags);
/* If even one was posted, there will be a completion. */
diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
index 90cba3058f04..6a87a2379e91 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
@@ -322,15 +322,17 @@ int svc_rdma_send(struct svcxprt_rdma *rdma, struct ib_send_wr *wr)
}
svc_xprt_get(&rdma->sc_xprt);
+ trace_svcrdma_post_send(wr);
ret = ib_post_send(rdma->sc_qp, wr, NULL);
- trace_svcrdma_post_send(wr, ret);
- if (ret) {
- set_bit(XPT_CLOSE, &rdma->sc_xprt.xpt_flags);
- svc_xprt_put(&rdma->sc_xprt);
- wake_up(&rdma->sc_send_wait);
- }
- break;
+ if (ret)
+ break;
+ return 0;
}
+
+ trace_svcrdma_sq_post_err(rdma, ret);
+ set_bit(XPT_CLOSE, &rdma->sc_xprt.xpt_flags);
+ svc_xprt_put(&rdma->sc_xprt);
+ wake_up(&rdma->sc_send_wait);
return ret;
}
Refactor: Instead of making two transport method calls in
svc_send(), fold the ->xpo_release_rqst call into the
->xpo_sendto method of the two transports that need this extra
behavior.
Subsequently, svcrdma, which does not want or need the extra
->xpo_release_rqst call can then use ->xpo_release_rqst properly.
This patch does not fix commit 3a88092ee319 ("svcrdma: Preserve
Receive buffer until svc_rdma_sendto"), but is a prerequisite for
the next patch, which does fix that commit.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/svc_xprt.c | 3 ---
net/sunrpc/svcsock.c | 4 ++++
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 92f2c08c67a5..2284ff038dad 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -908,9 +908,6 @@ int svc_send(struct svc_rqst *rqstp)
if (!xprt)
goto out;
- /* release the receive skb before sending the reply */
- xprt->xpt_ops->xpo_release_rqst(rqstp);
-
/* calculate over-all length */
xb = &rqstp->rq_res;
xb->len = xb->head[0].iov_len +
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 519cf9c4f8fd..023514e392b3 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -527,6 +527,8 @@ static int svc_udp_sendto(struct svc_rqst *rqstp)
unsigned int uninitialized_var(sent);
int err;
+ svc_release_udp_skb(rqstp);
+
svc_set_cmsg_data(rqstp, cmh);
err = xprt_sock_sendmsg(svsk->sk_sock, &msg, xdr, 0, 0, &sent);
@@ -1076,6 +1078,8 @@ static int svc_tcp_sendto(struct svc_rqst *rqstp)
unsigned int uninitialized_var(sent);
int err;
+ svc_release_skb(rqstp);
+
err = xprt_sock_sendmsg(svsk->sk_sock, &msg, xdr, 0, marker, &sent);
xdr_free_bvec(xdr);
if (err < 0 || sent != (xdr->len + sizeof(marker)))
Utilize the xpo_release_rqst transport method to ensure that each
rqstp's svc_rdma_recv_ctxt object is released even when the server
cannot return a Reply for that rqstp.
Without this fix, each RPC whose Reply cannot be sent leaks one
svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped
Receive buffer, and any pages that might be part of the Reply
message.
The leak is infrequent unless the network fabric is unreliable or
Kerberos is in use, as GSS sequence window overruns, which result
in connection loss, are more common on fast transports.
Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
Signed-off-by: Chuck Lever <[email protected]>
---
include/linux/sunrpc/svc_rdma.h | 1 +
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++
net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++----------
net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 -----
4 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
index 78fe2ac6dc6c..cbcfbd0521e3 100644
--- a/include/linux/sunrpc/svc_rdma.h
+++ b/include/linux/sunrpc/svc_rdma.h
@@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma);
extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
struct svc_rdma_recv_ctxt *ctxt);
extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma);
+extern void svc_rdma_release_rqst(struct svc_rqst *rqstp);
extern int svc_rdma_recvfrom(struct svc_rqst *);
/* svc_rdma_rw.c */
diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
index 54469b72b25f..efa5fcb5793f 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
@@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
svc_rdma_recv_ctxt_destroy(rdma, ctxt);
}
+/**
+ * svc_rdma_release_rqst - Release transport-specific per-rqst resources
+ * @rqstp: svc_rqst being released
+ *
+ * Ensure that the recv_ctxt is released whether or not a Reply
+ * was sent. For example, the client could close the connection,
+ * or svc_process could drop an RPC, before the Reply is sent.
+ */
+void svc_rdma_release_rqst(struct svc_rqst *rqstp)
+{
+ struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt;
+ struct svc_xprt *xprt = rqstp->rq_xprt;
+ struct svcxprt_rdma *rdma =
+ container_of(xprt, struct svcxprt_rdma, sc_xprt);
+
+ rqstp->rq_xprt_ctxt = NULL;
+ if (ctxt)
+ svc_rdma_recv_ctxt_put(rdma, ctxt);
+}
+
static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma,
struct svc_rdma_recv_ctxt *ctxt)
{
@@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
__be32 *p;
int ret;
+ rqstp->rq_xprt_ctxt = NULL;
+
spin_lock(&rdma_xprt->sc_rq_dto_lock);
ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q);
if (ctxt) {
diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
index 6a87a2379e91..b6c8643867f2 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
@@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp);
if (ret < 0)
goto err1;
- ret = 0;
-
-out:
- rqstp->rq_xprt_ctxt = NULL;
- svc_rdma_recv_ctxt_put(rdma, rctxt);
- return ret;
+ return 0;
err2:
if (ret != -E2BIG && ret != -EINVAL)
@@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp);
if (ret < 0)
goto err1;
- ret = 0;
- goto out;
+ return 0;
err1:
svc_rdma_send_ctxt_put(rdma, sctxt);
err0:
trace_svcrdma_send_failed(rqstp, ret);
set_bit(XPT_CLOSE, &xprt->xpt_flags);
- ret = -ENOTCONN;
- goto out;
+ return -ENOTCONN;
}
/**
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 8bb99980ae85..ea54785db4f8 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
struct sockaddr *sa, int salen,
int flags);
static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt);
-static void svc_rdma_release_rqst(struct svc_rqst *);
static void svc_rdma_detach(struct svc_xprt *xprt);
static void svc_rdma_free(struct svc_xprt *xprt);
static int svc_rdma_has_wspace(struct svc_xprt *xprt);
@@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
return NULL;
}
-static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
-{
-}
-
/*
* When connected, an svc_xprt has at least two references:
*
On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote:
> Utilize the xpo_release_rqst transport method to ensure that each
> rqstp's svc_rdma_recv_ctxt object is released even when the server
> cannot return a Reply for that rqstp.
>
> Without this fix, each RPC whose Reply cannot be sent leaks one
> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped
> Receive buffer, and any pages that might be part of the Reply
> message.
>
> The leak is infrequent unless the network fabric is unreliable or
> Kerberos is in use, as GSS sequence window overruns, which result
> in connection loss, are more common on fast transports.
>
> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
Chuck,
Can you please don't mangle the Fixes line?
A lot of automatization is relying on the fact that this line is canonical,
both in format and in the actual content.
Thanks
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> include/linux/sunrpc/svc_rdma.h | 1 +
> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++
> net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++----------
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 -----
> 4 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
> index 78fe2ac6dc6c..cbcfbd0521e3 100644
> --- a/include/linux/sunrpc/svc_rdma.h
> +++ b/include/linux/sunrpc/svc_rdma.h
> @@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma);
> extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
> struct svc_rdma_recv_ctxt *ctxt);
> extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma);
> +extern void svc_rdma_release_rqst(struct svc_rqst *rqstp);
> extern int svc_rdma_recvfrom(struct svc_rqst *);
>
> /* svc_rdma_rw.c */
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index 54469b72b25f..efa5fcb5793f 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
> svc_rdma_recv_ctxt_destroy(rdma, ctxt);
> }
>
> +/**
> + * svc_rdma_release_rqst - Release transport-specific per-rqst resources
> + * @rqstp: svc_rqst being released
> + *
> + * Ensure that the recv_ctxt is released whether or not a Reply
> + * was sent. For example, the client could close the connection,
> + * or svc_process could drop an RPC, before the Reply is sent.
> + */
> +void svc_rdma_release_rqst(struct svc_rqst *rqstp)
> +{
> + struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt;
> + struct svc_xprt *xprt = rqstp->rq_xprt;
> + struct svcxprt_rdma *rdma =
> + container_of(xprt, struct svcxprt_rdma, sc_xprt);
> +
> + rqstp->rq_xprt_ctxt = NULL;
> + if (ctxt)
> + svc_rdma_recv_ctxt_put(rdma, ctxt);
> +}
> +
> static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma,
> struct svc_rdma_recv_ctxt *ctxt)
> {
> @@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
> __be32 *p;
> int ret;
>
> + rqstp->rq_xprt_ctxt = NULL;
> +
> spin_lock(&rdma_xprt->sc_rq_dto_lock);
> ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q);
> if (ctxt) {
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> index 6a87a2379e91..b6c8643867f2 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> @@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
> ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp);
> if (ret < 0)
> goto err1;
> - ret = 0;
> -
> -out:
> - rqstp->rq_xprt_ctxt = NULL;
> - svc_rdma_recv_ctxt_put(rdma, rctxt);
> - return ret;
> + return 0;
>
> err2:
> if (ret != -E2BIG && ret != -EINVAL)
> @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
> ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp);
> if (ret < 0)
> goto err1;
> - ret = 0;
> - goto out;
> + return 0;
>
> err1:
> svc_rdma_send_ctxt_put(rdma, sctxt);
> err0:
> trace_svcrdma_send_failed(rqstp, ret);
> set_bit(XPT_CLOSE, &xprt->xpt_flags);
> - ret = -ENOTCONN;
> - goto out;
> + return -ENOTCONN;
> }
>
> /**
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 8bb99980ae85..ea54785db4f8 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> struct sockaddr *sa, int salen,
> int flags);
> static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt);
> -static void svc_rdma_release_rqst(struct svc_rqst *);
> static void svc_rdma_detach(struct svc_xprt *xprt);
> static void svc_rdma_free(struct svc_xprt *xprt);
> static int svc_rdma_has_wspace(struct svc_xprt *xprt);
> @@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> return NULL;
> }
>
> -static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
> -{
> -}
> -
> /*
> * When connected, an svc_xprt has at least two references:
> *
>
Hi Leon-
> On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <[email protected]> wrote:
>
> On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote:
>> Utilize the xpo_release_rqst transport method to ensure that each
>> rqstp's svc_rdma_recv_ctxt object is released even when the server
>> cannot return a Reply for that rqstp.
>>
>> Without this fix, each RPC whose Reply cannot be sent leaks one
>> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped
>> Receive buffer, and any pages that might be part of the Reply
>> message.
>>
>> The leak is infrequent unless the network fabric is unreliable or
>> Kerberos is in use, as GSS sequence window overruns, which result
>> in connection loss, are more common on fast transports.
>>
>> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
>
> Chuck,
>
> Can you please don't mangle the Fixes line?
I've read e-mail from others that advocate this form of mangling
instead of using commit message lines that are too long.
> A lot of automatization is relying on the fact that this line is canonical,
> both in format and in the actual content.
Understood, but checkpatch.pl does not complain about it. Perhaps,
therefore, it is the automation that is not correct.
The commit ID is what automation should key off of. The short
description is only for human consumption. In fact, the short
description can easily be reconstituted from the commit ID. The
reverse cannot be done reliably.
I'm not interested in bike-shedding this, however, so I will try
to remember to use complete short descriptions when posting to
linux-rdma.
> Thanks
>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>> include/linux/sunrpc/svc_rdma.h | 1 +
>> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++
>> net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++----------
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 -----
>> 4 files changed, 26 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
>> index 78fe2ac6dc6c..cbcfbd0521e3 100644
>> --- a/include/linux/sunrpc/svc_rdma.h
>> +++ b/include/linux/sunrpc/svc_rdma.h
>> @@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma);
>> extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
>> struct svc_rdma_recv_ctxt *ctxt);
>> extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma);
>> +extern void svc_rdma_release_rqst(struct svc_rqst *rqstp);
>> extern int svc_rdma_recvfrom(struct svc_rqst *);
>>
>> /* svc_rdma_rw.c */
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> index 54469b72b25f..efa5fcb5793f 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> @@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
>> svc_rdma_recv_ctxt_destroy(rdma, ctxt);
>> }
>>
>> +/**
>> + * svc_rdma_release_rqst - Release transport-specific per-rqst resources
>> + * @rqstp: svc_rqst being released
>> + *
>> + * Ensure that the recv_ctxt is released whether or not a Reply
>> + * was sent. For example, the client could close the connection,
>> + * or svc_process could drop an RPC, before the Reply is sent.
>> + */
>> +void svc_rdma_release_rqst(struct svc_rqst *rqstp)
>> +{
>> + struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt;
>> + struct svc_xprt *xprt = rqstp->rq_xprt;
>> + struct svcxprt_rdma *rdma =
>> + container_of(xprt, struct svcxprt_rdma, sc_xprt);
>> +
>> + rqstp->rq_xprt_ctxt = NULL;
>> + if (ctxt)
>> + svc_rdma_recv_ctxt_put(rdma, ctxt);
>> +}
>> +
>> static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma,
>> struct svc_rdma_recv_ctxt *ctxt)
>> {
>> @@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>> __be32 *p;
>> int ret;
>>
>> + rqstp->rq_xprt_ctxt = NULL;
>> +
>> spin_lock(&rdma_xprt->sc_rq_dto_lock);
>> ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q);
>> if (ctxt) {
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
>> index 6a87a2379e91..b6c8643867f2 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
>> @@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
>> ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp);
>> if (ret < 0)
>> goto err1;
>> - ret = 0;
>> -
>> -out:
>> - rqstp->rq_xprt_ctxt = NULL;
>> - svc_rdma_recv_ctxt_put(rdma, rctxt);
>> - return ret;
>> + return 0;
>>
>> err2:
>> if (ret != -E2BIG && ret != -EINVAL)
>> @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
>> ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp);
>> if (ret < 0)
>> goto err1;
>> - ret = 0;
>> - goto out;
>> + return 0;
>>
>> err1:
>> svc_rdma_send_ctxt_put(rdma, sctxt);
>> err0:
>> trace_svcrdma_send_failed(rqstp, ret);
>> set_bit(XPT_CLOSE, &xprt->xpt_flags);
>> - ret = -ENOTCONN;
>> - goto out;
>> + return -ENOTCONN;
>> }
>>
>> /**
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 8bb99980ae85..ea54785db4f8 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>> struct sockaddr *sa, int salen,
>> int flags);
>> static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt);
>> -static void svc_rdma_release_rqst(struct svc_rqst *);
>> static void svc_rdma_detach(struct svc_xprt *xprt);
>> static void svc_rdma_free(struct svc_xprt *xprt);
>> static int svc_rdma_has_wspace(struct svc_xprt *xprt);
>> @@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
>> return NULL;
>> }
>>
>> -static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
>> -{
>> -}
>> -
>> /*
>> * When connected, an svc_xprt has at least two references:
>> *
--
Chuck Lever
On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> Hi Leon-
>
> > On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <[email protected]> wrote:
> >
> > On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote:
> >> Utilize the xpo_release_rqst transport method to ensure that each
> >> rqstp's svc_rdma_recv_ctxt object is released even when the server
> >> cannot return a Reply for that rqstp.
> >>
> >> Without this fix, each RPC whose Reply cannot be sent leaks one
> >> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped
> >> Receive buffer, and any pages that might be part of the Reply
> >> message.
> >>
> >> The leak is infrequent unless the network fabric is unreliable or
> >> Kerberos is in use, as GSS sequence window overruns, which result
> >> in connection loss, are more common on fast transports.
> >>
> >> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
> >
> > Chuck,
> >
> > Can you please don't mangle the Fixes line?
>
> I've read e-mail from others that advocate this form of mangling
> instead of using commit message lines that are too long.
Really?
At least I won't accept Fixes lines that are not in the cannonical
format, I routinely fix these things in all sorts of ways, but I've
never seen someone shorten it with ...
> > A lot of automatization is relying on the fact that this line is canonical,
> > both in format and in the actual content.
>
> Understood, but checkpatch.pl does not complain about it. Perhaps,
> therefore, it is the automation that is not correct.
checkpatch.pl doesn't check Fixes lines for correctness, because it
doesn't have access to the git or something. This was talked about
too.. Stephen likes to check them as part of linux-next though.
However, checkpatch.pl does not complain for long lines on Fixes:
tags demanding they be shortened
> The commit ID is what automation should key off of. The short
> description is only for human consumption.
Right, so if the actual commit message isn't included so humans can
read it then what was the point of including anything?
Jason
> On Apr 9, 2020, at 1:47 PM, Jason Gunthorpe <[email protected]> wrote:
>
> On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
>> Hi Leon-
>>
>>> On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <[email protected]> wrote:
>>>
>>> On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote:
>>>> Utilize the xpo_release_rqst transport method to ensure that each
>>>> rqstp's svc_rdma_recv_ctxt object is released even when the server
>>>> cannot return a Reply for that rqstp.
>>>>
>>>> Without this fix, each RPC whose Reply cannot be sent leaks one
>>>> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped
>>>> Receive buffer, and any pages that might be part of the Reply
>>>> message.
>>>>
>>>> The leak is infrequent unless the network fabric is unreliable or
>>>> Kerberos is in use, as GSS sequence window overruns, which result
>>>> in connection loss, are more common on fast transports.
>>>>
>>>> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
>>>
>>> Chuck,
>>>
>>> Can you please don't mangle the Fixes line?
>>
>> I've read e-mail from others that advocate this form of mangling
>> instead of using commit message lines that are too long.
>
> Really?
Yep.
> At least I won't accept Fixes lines that are not in the cannonical
> format, I routinely fix these things in all sorts of ways, but I've
> never seen someone shorten it with ...
It seems that is a Maintainers preference.
>>> A lot of automatization is relying on the fact that this line is canonical,
>>> both in format and in the actual content.
>>
>> Understood, but checkpatch.pl does not complain about it. Perhaps,
>> therefore, it is the automation that is not correct.
>
> checkpatch.pl doesn't check Fixes lines for correctness,
It certainly does check Fixes: lines for correctness. Lately, it's
been warning about commit IDs that are not exactly 12 hexits long,
and cases where the commit ID does not actually exist in the local
repository.
It used to complain about "..." as well, until someone said that is
a frequently-used modification to keep the line length manageable.
I think that might be why checkpatch.pl no longer checks the tag's
short description.
> because it
> doesn't have access to the git or something. This was talked about
> too.. Stephen likes to check them as part of linux-next though.
>
> However, checkpatch.pl does not complain for long lines on Fixes:
> tags demanding they be shortened
Maybe not, but I don't think that's always been the case. <shrug>
>> The commit ID is what automation should key off of. The short
>> description is only for human consumption.
>
> Right, so if the actual commit message isn't included so humans can
> read it then what was the point of including anything?
I didn't invent the Fixes: tag, and I didn't invent shortening it.
That had to come from somewhere as well. So here we are.
No matter, though. Now that I know this community's preference, I
will stick to it. Will you take "yes" for an answer? ;-)
--
Chuck Lever
On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > The commit ID is what automation should key off of. The short
> > > description is only for human consumption.
> >
> > Right, so if the actual commit message isn't included so humans can
> > read it then what was the point of including anything?
>
> Personally as a human reading commits in a terminal window I prefer the
> abbreviated form.
Frankly, I think they are useless, picking one of yours at random:
Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
now we are just totally lost, with a bad commit ID and a mangled
subject line.
> I haven't been doing the redundant parentheses and quotes either. Was
> that dreamt up by an Arlo Guthrie fan? ("KID, HAVE YOU REHABILITATED
> YOURSELF?")
Well it seems like you are just aren't following the standard style
at all. :(
Jason
On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote:
> On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote:
> > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > > > The commit ID is what automation should key off of. The short
> > > > > description is only for human consumption.
> > > >
> > > > Right, so if the actual commit message isn't included so humans can
> > > > read it then what was the point of including anything?
> > >
> > > Personally as a human reading commits in a terminal window I prefer the
> > > abbreviated form.
> >
> > Frankly, I think they are useless, picking one of yours at random:
> >
> > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
> >
> > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
>
> Ow, apologies. Looks like I rebased after writing that Fixes tag.
>
> I wonder if it's possible to make git warn....
>
> Looks like a pre-rebase hook could check the branch being rebased for
> "Fixes:" lines referencing commits on the rebased branch.
I have some silly stuff to check patches before pushing them and it
includes checking the fixes lines because they are very often
wrong, both with wrong commit IDs and wrong subjects!
linux-next now automates complaining about them, but perhaps not
following the standard format defeats that..
Use 'git merge-base --is-ancestor fixes_id linus/master' to check
them.
> > now we are just totally lost, with a bad commit ID and a mangled
> > subject line.
>
> For what it's worth, that part of the subject line is enough to find the
> original commit (even to uniquely specify it).
Lucky, but I wouldn't count on this as the general rule.. The point of
the full subject line is to be informative to the reader and serve as
a backup key in case the hash got mangled, as happens surprisingly
often..
Jason
On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > The commit ID is what automation should key off of. The short
> > description is only for human consumption.
>
> Right, so if the actual commit message isn't included so humans can
> read it then what was the point of including anything?
Personally as a human reading commits in a terminal window I prefer the
abbreviated form.
I haven't been doing the redundant parentheses and quotes either. Was
that dreamt up by an Arlo Guthrie fan? ("KID, HAVE YOU REHABILITATED
YOURSELF?")
--b.
On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > > The commit ID is what automation should key off of. The short
> > > > description is only for human consumption.
> > >
> > > Right, so if the actual commit message isn't included so humans can
> > > read it then what was the point of including anything?
> >
> > Personally as a human reading commits in a terminal window I prefer the
> > abbreviated form.
>
> Frankly, I think they are useless, picking one of yours at random:
>
> Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
>
> And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
Ow, apologies. Looks like I rebased after writing that Fixes tag.
I wonder if it's possible to make git warn....
Looks like a pre-rebase hook could check the branch being rebased for
"Fixes:" lines referencing commits on the rebased branch.
> now we are just totally lost, with a bad commit ID and a mangled
> subject line.
For what it's worth, that part of the subject line is enough to find the
original commit (even to uniquely specify it).
> > I haven't been doing the redundant parentheses and quotes either. Was
> > that dreamt up by an Arlo Guthrie fan? ("KID, HAVE YOU REHABILITATED
> > YOURSELF?")
>
> Well it seems like you are just aren't following the standard style
> at all. :(
Yeah, I don't like it.
I'll admit I don't know why *this* exactly is what I'm choosing to feel
stubborn about.
--b.
On Tue, Apr 14, 2020 at 12:17:44PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote:
> > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote:
> > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote:
> > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > > > > > The commit ID is what automation should key off of. The short
> > > > > > > description is only for human consumption.
> > > > > >
> > > > > > Right, so if the actual commit message isn't included so humans can
> > > > > > read it then what was the point of including anything?
> > > > >
> > > > > Personally as a human reading commits in a terminal window I prefer the
> > > > > abbreviated form.
> > > >
> > > > Frankly, I think they are useless, picking one of yours at random:
> > > >
> > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
> > > >
> > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
> > >
> > > Ow, apologies. Looks like I rebased after writing that Fixes tag.
> > >
> > > I wonder if it's possible to make git warn....
> > >
> > > Looks like a pre-rebase hook could check the branch being rebased for
> > > "Fixes:" lines referencing commits on the rebased branch.
> >
> > I have some silly stuff to check patches before pushing them and it
> > includes checking the fixes lines because they are very often
> > wrong, both with wrong commit IDs and wrong subjects!
>
> I'd be interested in seeing it.
checkpatch.pl checks it or supposed to check.
commit a8dd86bf746256fbf68f82bc13356244c5ad8efa
Author: Matteo Croce <[email protected]>
Date: Wed Sep 25 16:46:38 2019 -0700
checkpatch.pl: warn on invalid commit id
It can happen that a commit message refers to an invalid commit id,
because the referenced hash changed following a rebase, or simply by
mistake. Add a check in checkpatch.pl which checks that an hash
referenced by a Fixes tag, or just cited in the commit message, is a valid
commit id.
$ scripts/checkpatch.pl <<'EOF'
Subject: [PATCH] test commit
Sample test commit to test checkpatch.pl
Commit 1da177e4c3f4 ("Linux-2.6.12-rc2") really exists,
commit 0bba044c4ce7 ("tree") is valid but not a commit,
while commit b4cc0b1c0cca ("unknown") is invalid.
Fixes: f0cacc14cade ("unknown")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
EOF
WARNING: Unknown commit id '0bba044c4ce7', maybe rebased or not pulled?
#8:
commit 0bba044c4ce7 ("tree") is valid but not a commit,
WARNING: Unknown commit id 'b4cc0b1c0cca', maybe rebased or not pulled?
#9:
while commit b4cc0b1c0cca ("unknown") is invalid.
WARNING: Unknown commit id 'f0cacc14cade', maybe rebased or not pulled?
#11:
Fixes: f0cacc14cade ("unknown")
total: 0 errors, 3 warnings, 4 lines checked
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Matteo Croce <[email protected]>
Cc: Joe Perches <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
On Tue, Apr 14, 2020 at 09:11:41PM +0300, Leon Romanovsky wrote:
> On Tue, Apr 14, 2020 at 12:17:44PM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote:
> > > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote:
> > > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote:
> > > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> > > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > > > > > > The commit ID is what automation should key off of. The short
> > > > > > > > description is only for human consumption.
> > > > > > >
> > > > > > > Right, so if the actual commit message isn't included so humans can
> > > > > > > read it then what was the point of including anything?
> > > > > >
> > > > > > Personally as a human reading commits in a terminal window I prefer the
> > > > > > abbreviated form.
> > > > >
> > > > > Frankly, I think they are useless, picking one of yours at random:
> > > > >
> > > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
> > > > >
> > > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
> > > >
> > > > Ow, apologies. Looks like I rebased after writing that Fixes tag.
> > > >
> > > > I wonder if it's possible to make git warn....
> > > >
> > > > Looks like a pre-rebase hook could check the branch being rebased for
> > > > "Fixes:" lines referencing commits on the rebased branch.
> > >
> > > I have some silly stuff to check patches before pushing them and it
> > > includes checking the fixes lines because they are very often
> > > wrong, both with wrong commit IDs and wrong subjects!
> >
> > I'd be interested in seeing it.
>
> checkpatch.pl checks it or supposed to check.
This doesn't do the --is-ancestor check, so it is quite weak. It would
not have triggered for Chuck because he'd still have the commit in his
local git from the rebase.
Jason
On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote:
> On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote:
> > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote:
> > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > > > > The commit ID is what automation should key off of. The short
> > > > > > description is only for human consumption.
> > > > >
> > > > > Right, so if the actual commit message isn't included so humans can
> > > > > read it then what was the point of including anything?
> > > >
> > > > Personally as a human reading commits in a terminal window I prefer the
> > > > abbreviated form.
> > >
> > > Frankly, I think they are useless, picking one of yours at random:
> > >
> > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
> > >
> > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
> >
> > Ow, apologies. Looks like I rebased after writing that Fixes tag.
> >
> > I wonder if it's possible to make git warn....
> >
> > Looks like a pre-rebase hook could check the branch being rebased for
> > "Fixes:" lines referencing commits on the rebased branch.
>
> I have some silly stuff to check patches before pushing them and it
> includes checking the fixes lines because they are very often
> wrong, both with wrong commit IDs and wrong subjects!
I'd be interested in seeing it.
> linux-next now automates complaining about them, but perhaps not
> following the standard format defeats that..
It's managed before:
https://lore.kernel.org/r/[email protected]
though admittedly I was breaking the rule in a different way. I can't
even be consistently rebellious.
> Use 'git merge-base --is-ancestor fixes_id linus/master' to check
> them.
Oh, yeah, that's better than what I was trying to do, thanks.
--b.
On Tue, Apr 14, 2020 at 12:17:44PM -0400, J. Bruce Fields wrote:
> On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote:
> > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote:
> > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote:
> > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote:
> > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote:
> > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote:
> > > > > > > The commit ID is what automation should key off of. The short
> > > > > > > description is only for human consumption.
> > > > > >
> > > > > > Right, so if the actual commit message isn't included so humans can
> > > > > > read it then what was the point of including anything?
> > > > >
> > > > > Personally as a human reading commits in a terminal window I prefer the
> > > > > abbreviated form.
> > > >
> > > > Frankly, I think they are useless, picking one of yours at random:
> > > >
> > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... "
> > > >
> > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so
> > >
> > > Ow, apologies. Looks like I rebased after writing that Fixes tag.
> > >
> > > I wonder if it's possible to make git warn....
> > >
> > > Looks like a pre-rebase hook could check the branch being rebased for
> > > "Fixes:" lines referencing commits on the rebased branch.
> >
> > I have some silly stuff to check patches before pushing them and it
> > includes checking the fixes lines because they are very often
> > wrong, both with wrong commit IDs and wrong subjects!
>
> I'd be interested in seeing it.
Sure, let me put my scripts on github..
Jason