The nfs code uses data provided to the packet handler after net_poll() returned. But the life time of this data already ended when net_poll() returns. Most of the time it is possible to get away here but on i.MX28 the data is overwritten since commit 82ec28929cc9 ("net: fec_imx: Do not use DMA coherent memory for Rx buffers"). So the data from the packet is copied to a malloced buffer. To ease tracking the lifetime of this buffer let rpc_req return the packet data and the caller free it when done using it. This reduces usage of global data considerably and so makes the code easier to follow. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> --- Hello, this is a more robust fix for 82ec28929cc9 breaking nfs on i.MX28 than reported in the mail with Message-ID: <20190114141717.nbs3h3nxnrrbizob@xxxxxxxxxxxxxx>. Best regards Uwe fs/nfs.c | 137 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 82 insertions(+), 55 deletions(-) diff --git a/fs/nfs.c b/fs/nfs.c index d7f156687fc9..0fad28af331b 100644 --- a/fs/nfs.c +++ b/fs/nfs.c @@ -102,8 +102,12 @@ #define NFS3ERR_BADTYPE 10007 #define NFS3ERR_JUKEBOX 10008 -static void *nfs_packet; -static int nfs_len; +struct packet { + int len; + char data[]; +}; + +static struct packet *nfs_packet; struct rpc_call { uint32_t id; @@ -355,8 +359,8 @@ static uint32_t *rpc_add_credentials(uint32_t *p) return p; } -static int rpc_check_reply(unsigned char *pkt, - int rpc_prog, uint32_t rpc_id, int *nfserr) +static int rpc_check_reply(struct packet *pkt, int rpc_prog, + uint32_t rpc_id, int *nfserr) { uint32_t *data; struct rpc_reply rpc; @@ -366,7 +370,7 @@ static int rpc_check_reply(unsigned char *pkt, if (!pkt) return -EAGAIN; - memcpy(&rpc, pkt, sizeof(rpc)); + memcpy(&rpc, pkt->data, sizeof(rpc)); if (ntoh32(rpc.id) != rpc_id) { if (rpc_id - ntoh32(rpc.id) == 1) @@ -385,7 +389,7 @@ static int rpc_check_reply(unsigned char *pkt, if (rpc_prog == PROG_PORTMAP) return 0; - data = (uint32_t *)(pkt + sizeof(struct rpc_reply)); + data = (uint32_t *)(pkt->data + sizeof(struct rpc_reply)); *nfserr = ntoh32(net_read_uint32(data)); *nfserr = -*nfserr; @@ -397,8 +401,8 @@ static int rpc_check_reply(unsigned char *pkt, /* * rpc_req - synchronous RPC request */ -static int rpc_req(struct nfs_priv *npriv, int rpc_prog, int rpc_proc, - uint32_t *data, int datalen) +static struct packet *rpc_req(struct nfs_priv *npriv, int rpc_prog, + int rpc_proc, uint32_t *data, int datalen) { struct rpc_call pkt; unsigned short dport; @@ -440,31 +444,32 @@ again: nfs_timer_start = get_time_ns(); nfs_state = STATE_START; - nfs_packet = NULL; while (nfs_state != STATE_DONE) { - if (ctrlc()) { - ret = -EINTR; - break; - } + if (ctrlc()) + return ERR_PTR(-EINTR); + net_poll(); if (is_timeout(nfs_timer_start, NFS_TIMEOUT)) { tries++; if (tries == NFS_MAX_RESEND) - return -ETIMEDOUT; + return ERR_PTR(-ETIMEDOUT); goto again; } ret = rpc_check_reply(nfs_packet, rpc_prog, - npriv->rpc_id, &nfserr); + npriv->rpc_id, &nfserr); if (!ret) { - ret = nfserr; - break; + if (nfserr) { + free(nfs_packet); + return ERR_PTR(nfserr); + } else + return nfs_packet; } } - return ret; + return nfs_packet; } /* @@ -473,7 +478,7 @@ again: static int rpc_lookup_req(struct nfs_priv *npriv, uint32_t prog, uint32_t ver) { uint32_t data[16]; - int ret; + struct packet *packet; uint32_t port; data[0] = 0; data[1] = 0; /* auth credential */ @@ -483,11 +488,14 @@ static int rpc_lookup_req(struct nfs_priv *npriv, uint32_t prog, uint32_t ver) data[6] = hton32(17); /* IP_UDP */ data[7] = 0; - ret = rpc_req(npriv, PROG_PORTMAP, PORTMAP_GETPORT, data, 8); - if (ret) - return ret; + packet = rpc_req(npriv, PROG_PORTMAP, PORTMAP_GETPORT, data, 8); + if (IS_ERR(packet)) + return PTR_ERR(packet); + + port = ntoh32(net_read_uint32(packet->data + sizeof(struct rpc_reply))); + + free(packet); - port = ntoh32(net_read_uint32(nfs_packet + sizeof(struct rpc_reply))); return port; } @@ -629,7 +637,7 @@ static int nfs_mount_req(struct nfs_priv *npriv) uint32_t *p; int len; int pathlen; - int ret; + struct packet *packet; pathlen = strlen(npriv->path); @@ -647,20 +655,22 @@ static int nfs_mount_req(struct nfs_priv *npriv) len = p - &(data[0]); - ret = rpc_req(npriv, PROG_MOUNT, MOUNT_ADDENTRY, data, len); - if (ret) - return ret; + packet = rpc_req(npriv, PROG_MOUNT, MOUNT_ADDENTRY, data, len); + if (IS_ERR(packet)) + return PTR_ERR(packet); - p = nfs_packet + sizeof(struct rpc_reply) + 4; + p = (void *)packet->data + sizeof(struct rpc_reply) + 4; npriv->rootfh.size = ntoh32(net_read_uint32(p++)); if (npriv->rootfh.size > NFS3_FHSIZE) { - printf("%s: file handle too big: %lu\n", __func__, - (unsigned long)npriv->rootfh.size); + printf("%s: file handle too big: %lu\n", + __func__, (unsigned long)npriv->rootfh.size); + free(packet); return -EIO; } memcpy(npriv->rootfh.data, p, npriv->rootfh.size); - p += DIV_ROUND_UP(npriv->rootfh.size, 4); + + free(packet); return 0; } @@ -674,6 +684,7 @@ static void nfs_umount_req(struct nfs_priv *npriv) uint32_t *p; int len; int pathlen; + struct packet *packet; pathlen = strlen(npriv->path); @@ -684,7 +695,10 @@ static void nfs_umount_req(struct nfs_priv *npriv) len = p - &(data[0]); - rpc_req(npriv, PROG_MOUNT, MOUNT_UMOUNT, data, len); + packet = rpc_req(npriv, PROG_MOUNT, MOUNT_UMOUNT, data, len); + + if (!IS_ERR(packet)) + free(packet); } /* @@ -699,7 +713,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh, uint32_t data[1024]; uint32_t *p; int len; - int ret; + struct packet *packet; /* * struct LOOKUP3args { @@ -735,11 +749,11 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh, len = p - &(data[0]); - ret = rpc_req(npriv, PROG_NFS, NFSPROC3_LOOKUP, data, len); - if (ret) - return ret; + packet = rpc_req(npriv, PROG_NFS, NFSPROC3_LOOKUP, data, len); + if (IS_ERR(packet)) + return PTR_ERR(packet); - p = nfs_packet + sizeof(struct rpc_reply) + 4; + p = (void *)packet->data + sizeof(struct rpc_reply) + 4; ninode->fh.size = ntoh32(net_read_uint32(p++)); if (ninode->fh.size > NFS3_FHSIZE) { @@ -752,6 +766,8 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh, nfs_read_post_op_attr(p, inode); + free(packet); + return 0; } @@ -764,7 +780,7 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir) uint32_t data[1024]; uint32_t *p; int len; - int ret; + struct packet *packet; void *buf; /* @@ -816,20 +832,21 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir) p = nfs_add_uint32(p, 1024); /* count */ - ret = rpc_req(npriv, PROG_NFS, NFSPROC3_READDIR, data, p - data); - if (ret) + packet = rpc_req(npriv, PROG_NFS, NFSPROC3_READDIR, data, p - data); + if (IS_ERR(packet)) return NULL; - p = nfs_packet + sizeof(struct rpc_reply) + 4; + p = (void *)packet->data + sizeof(struct rpc_reply) + 4; p = nfs_read_post_op_attr(p, NULL); /* update cookieverf */ memcpy(dir->cookieverf, p, NFS3_COOKIEVERFSIZE); p += NFS3_COOKIEVERFSIZE / 4; - len = nfs_packet + nfs_len - (void *)p; + len = (void *)packet->data + packet->len - (void *)p; if (!len) { printf("%s: huh, no payload left\n", __func__); + free(packet); return NULL; } @@ -837,6 +854,8 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir) memcpy(buf, p, len); + free(packet); + xdr_init(&dir->stream, buf, len); /* now xdr points to dirlist3 res.resok.reply */ @@ -853,7 +872,7 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset, uint32_t data[1024]; uint32_t *p; int len; - int ret; + struct packet *packet; uint32_t rlen, eof; /* @@ -890,11 +909,11 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset, len = p - &(data[0]); - ret = rpc_req(priv->npriv, PROG_NFS, NFSPROC3_READ, data, len); - if (ret) - return ret; + packet = rpc_req(priv->npriv, PROG_NFS, NFSPROC3_READ, data, len); + if (IS_ERR(packet)) + return PTR_ERR(packet); - p = nfs_packet + sizeof(struct rpc_reply) + 4; + p = (void *)packet->data + sizeof(struct rpc_reply) + 4; p = nfs_read_post_op_attr(p, NULL); @@ -911,11 +930,15 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset, */ p += 2; - if (readlen && !rlen && !eof) + if (readlen && !rlen && !eof) { + free(packet); return -EIO; + } kfifo_put(priv->fifo, (char *)p, rlen); + free(packet); + return 0; } @@ -924,8 +947,10 @@ static void nfs_handler(void *ctx, char *packet, unsigned len) char *pkt = net_eth_to_udp_payload(packet); nfs_state = STATE_DONE; - nfs_packet = pkt; - nfs_len = len; + + nfs_packet = xmalloc(sizeof(*nfs_packet) + len); + memcpy(nfs_packet->data, pkt, len); + nfs_packet->len = len; } static int nfs_truncate(struct device_d *dev, FILE *f, ulong size) @@ -947,7 +972,7 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh, uint32_t data[1024]; uint32_t *p; uint32_t len; - int ret; + struct packet *packet; /* * struct READLINK3args { @@ -977,11 +1002,11 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh, len = p - &(data[0]); - ret = rpc_req(npriv, PROG_NFS, NFSPROC3_READLINK, data, len); - if (ret) - return ret; + packet = rpc_req(npriv, PROG_NFS, NFSPROC3_READLINK, data, len); + if (IS_ERR(packet)) + return PTR_ERR(packet); - p = nfs_packet + sizeof(struct rpc_reply) + 4; + p = (void *)packet->data + sizeof(struct rpc_reply) + 4; p = nfs_read_post_op_attr(p, NULL); @@ -991,6 +1016,8 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh, *target = xzalloc(len + 1); memcpy(*target, p, len); + free(packet); + return 0; } -- 2.20.1 _______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox