On 20.06.23 09:39, Wu, Wentong wrote:
Hi,
+
+ ibuf_len = READ_ONCE(stub->ipacket.ibuf_len);
+ ibuf = READ_ONCE(stub->ipacket.ibuf);
This races against stub_write(). Yes, every value now is consistent within this
function. But that does not solve the issue. The pair needs to be consistent. You
need to rule out that you are reading a different length and buffer location. I am
afraid this needs a spinlock.
Sorry, based on my understanding, the stub's ibuf_len and ibuf won't be changed by
stub_write until firmware ack the current command.
Or a timeout.
There are two places in the driver ibuf_len and ibuf are touched.
Both are in ljca_stub_write(), but in case of a timeout they are set
to NULL. And you are checking for that case. That is good, but not sufficient,
as READ_ONCE does not guarantee ordering, nor do you use WRITE_ONCE to set them.
This memory barrier here is for potential cache consistency issue in some arches though
most likely this driver will only be run on X86 platform.
Well, you can move them to architecture dependent directories. But if not, they
will have to work generically.
A correct version would be something like:
+static int ljca_parse(struct ljca_dev *dev, struct ljca_msg *header)
+{
+ struct ljca_stub *stub;
+ unsigned int *ibuf_len;
+ u8 *ibuf;
+
+ stub = ljca_stub_find(dev, header->type);
+ if (IS_ERR(stub))
+ return PTR_ERR(stub);
+
+ if (!(header->flags & LJCA_ACK_FLAG)) {
+ ljca_stub_notify(stub, header->cmd, header->data, header->len);
+ return 0;
+ }
+
+ if (stub->cur_cmd != header->cmd) {
+ dev_err(&dev->intf->dev, "header and stub current command mismatch (%x vs %x)\n",
+ header->cmd, stub->cur_cmd);
+ return -EINVAL;
+ }
+
smp_rmb();
+ ibuf_len = READ_ONCE(stub->ipacket.ibuf_len);
+ ibuf = READ_ONCE(stub->ipacket.ibuf);
+
+ if (ibuf && ibuf_len) {
+ unsigned int newlen;
+
+ newlen = min_t(unsigned int, header->len, *ibuf_len);
+
+ *ibuf_len = newlen;
+ memcpy(ibuf, header->data, newlen);
+ }
+
+ stub->acked = true;
+ wake_up(&dev->ack_wq);
+
+ return 0;
+}
+
+static int ljca_stub_write(struct ljca_stub *stub, u8 cmd, const void *obuf, unsigned int obuf_len,
+ void *ibuf, unsigned int *ibuf_len, bool wait_ack, unsigned long timeout)
+{
+ struct ljca_dev *dev = usb_get_intfdata(stub->intf);
+ u8 flags = LJCA_CMPL_FLAG;
+ struct ljca_msg *header;
+ unsigned int msg_len = sizeof(*header) + obuf_len;
+ int actual;
+ int ret;
+
+ if (msg_len > LJCA_MAX_PACKET_SIZE)
+ return -EINVAL;
+
+ if (wait_ack)
+ flags |= LJCA_ACK_FLAG;
+
+ header = kmalloc(msg_len, GFP_KERNEL);
+ if (!header)
+ return -ENOMEM;
+
+ header->type = stub->type;
+ header->cmd = cmd;
+ header->flags = flags;
+ header->len = obuf_len;
+
+ if (obuf)
+ memcpy(header->data, obuf, obuf_len);
+
+ dev_dbg(&dev->intf->dev, "send: type:%d cmd:%d flags:%d len:%d\n", header->type,
+ header->cmd, header->flags, header->len);
+
+ usb_autopm_get_interface(dev->intf);
+ if (!dev->started) {
+ kfree(header);
+ ret = -ENODEV;
+ goto error_put;
+ }
+
+ mutex_lock(&dev->mutex);
+ stub->cur_cmd = cmd;
WRITE_ONCE(stub->ipacket.ibuf, ibuf);
WRITE_ONCE(stub->ipacket.ibuf_len, ibuf_len);
+ stub->acked = false;
smp_wmb();
[..]
+error_unlock:
WRITE_ONCE(stub->ipacket.ibuf, NULL);
WRITE_ONCE(stub->ipacket.ibuf_len, NULL);
smp_wmb();
+ mutex_unlock(&dev->mutex);
Regards
Oliver
